Add getFragment method to ReactLocalization #595

Vinnl · 2022-07-09T11:53:25Z

Fixes #544.

It's similar to getString, but returns a React Fragment instead
of a string. The main added benefit here is that it allows the
caller to pass in elems analogous to the similarly-named prop on
<Localized>.

We're mostly using getString in the Firefox Relay codebase, but have to switch to <Localized> every time we want to use an element in the translation string, which can be confusing to new contributors.

I do understand that generally it's best to be aligned on a new feature before submitting a PR, which I didn't do, so I want to emphasise that it's perfectly fine to let me know that this feature isn't desirable for one reason or another, or that I should redo everything if I want to get this in. It didn't seem like a feature that was too hard to implement, so I decided to just go ahead and give it a shot, with the worst case being a little wasted effort :)

It's similar to getString(), but returns a React Fragment instead of a string. The main added benefit here is that it allows the caller to pass in `elems` analogous to the similarly-named prop on <Localized>.

eemeli · 2022-08-03T11:28:49Z

Apologies for the delay; I've been travelling and currently fluent.js reviews pretty much fall on me.

In general, I think I'd be fine with adding getFragment(), as it makes behaviour that's now only available via Localized usable from Localization as well.

I took a quick look at this PR; do I understand right that effectively you've combined code from getString() in Localization with the elems and message attribute handling of Localized in order to create getFragment()? My main issue with this PR is that it ends up duplicating quite a bit of code, and I'd much rather have a single internal implementation that's called from two different places for that.

Vinnl · 2022-08-03T11:38:25Z

No worries @eemeli, hope you enjoyed your travels :)

Yes, you are correct about the approach I took. I understand the desire for a single internal implementation, but I'm hesitant about doing that myself - I find that introducing layers of abstraction ideally is informed by an idea of which lines of code are likely to change together in the future, which is hard to do for someone like me who's unfamiliar with the codebase and its history. The wrong abstractions are often worse than no abstraction, and I don't want to be responsible for that :)

Possibly I could give it a shot if you'd have some pointers about the abstractions you'd like to see?

eemeli · 2022-08-03T14:32:33Z

@Vinnl I'd be happy to have you poke at this. :)

My gut feeling would be to have it so that Localized calls getFragment() internally. If there's a reason why that wouldn't be a good idea, you'll probably find it on the way. 😇

Vinnl · 2022-09-01T15:54:09Z

Thanks @eemeli, that made sense. There was a bunch of extra code in <Localized> dealing with its children and setting attributes. I've now added a getElement function that replicates that functionality, redefined getFragment to just call that, and redefined <Localized> to use it as well. That should cut down on a lot of similar code - let me know what you think.

(And apologies for the delayed response - holidays and other work and all that :) )

eemeli

I like this! I do think we need to bikeshed the API a bit, though. See inline comments.

fluent-react/src/localization.ts

eemeli · 2022-09-05T06:22:52Z

fluent-react/src/localization.ts

+    args?: {
+      vars?: Record<string, FluentVariable>,
+      elems?: Record<string, ReactElement>,
+      attrs?: Record<string, boolean>;
+    },


I'm not really sure about this. In <Localized/> passing each of these in as props makes sense, but here it feels like we're putting different things into the same basket -- esp. as this is so close to the shape of the getString() args argument.

My first instinct would be to separate vars from the other two, but maybe calling this something other than args could also help?

Oh heh, good eye. I'd actually argue that renaming getStrings second argument to vars would be more appropriate, since the Fluent docs also call them variables, but I can understand that changing the name of an existing API might not be desirable.

I could name it something like props? Because they're basically analogous to <Localized>'s props, although it could also be confusing, given that they're not React props. Or how about params?

I'm ok with changing the internal name of the getSrting() argument to vars, because it only impacts the documentation.

props and params aren't really better, and both come with their own sets of baggage.

fluent-react/src/localization.ts

eemeli · 2022-09-05T06:39:23Z

fluent-react/src/localization.ts

+    // Check if the component to render is a valid element -- if not, then
+    // it's either null or a simple fallback string. No need to localize the
+    // attributes.
+    if (!isValidElement(componentToRender)) {


I would really rather prefer requiring componentToRender to be a valid element when using this method. For <Localized/> we do need to continue supporting use with no child, but would it be possible to check for that and handle it there, rather than needing to do so here?

Hmm, I'm not sure what the best way to do that is - there'd be quite a bit of duplication in initialising the bundle and handling its various error cases. Personally I wouldn't be too worried about doing too much error handling. What I could do is update the type definition of getElement to only allow ReactChild | ReactFragment? Then we're technically requiring it to be a valid element in the public API.

Or, hmm, what I could also do is update <Localized> to check for non-element children and, if found, wrap them in a fragment and pass that to getElement? That would cause the latter to do some unnecessary work, but nothing too egregious I thinkg - what do you think?

How about calling getString() rather than getElement() from a Localized with non-element children, and wrapping the result in a Fragment? As far as I can tell, that should replicate the current behaviour.

Ah yeah, that's clever: 59671cc

I should note that that does change the behaviour a little bit: if you do not provide a child, it will now render the string ID, rather than null. Which makes sense to me and aligns it with the existing behaviour of getString(), but I can also imagine that that's not desirable.

One way to work around that might be to set the fallback string to some unique string, and return null instead of a fragment if getString returns the fallback string, but that feels pretty hacky :/ Alternatively, although still a change in behaviour but closer to the previous behaviour, would be to set the fallback string to the empty string, resulting in it returning a fragment containing an empty string instead of null. That's technically different from what it used to be, but should have the same result for the user.

fluent-react/src/localization.ts

As per projectfluent#595 (comment)

eemeli · 2022-09-09T12:18:13Z

fluent-react/src/localization.ts

@@ -106,7 +106,7 @@ export class ReactLocalization {
    if (Array.isArray(component)) {
      if (component.length > 1) {
        throw new Error(
-          "<Localized/> expected to receive a single React node child"
+          "Expected to receive a single React node element to inject a localized string into."


Suggested change

"Expected to receive a single React node element to inject a localized string into."

"Expected to receive a single React element to localize"

Adding more words doesn't really help here.

fluent-react/src/localization.ts

eemeli

See replies above for requested changes, and a couple minor new inline comments.

It's a thin wrapper, so not worth it for now to spend additional API surface area on, as per projectfluent#595 (comment)

It's equivalent to the `vars` prop on <Localiszed> and the respective parameter for getElement, holding Fluent variables.

As per projectfluent#595 (comment)

Vinnl · 2022-10-20T21:40:59Z

@eemeli Just wondering if you have time to look at my latest changes - I think I've addressed all your comments :)

eemeli

@Vinnl Sorry for the delay here, finally managed to take a look. A few small changes, and then I think we're good?

fluent-react/src/localized.ts

fluent-react/src/localization.ts

As per projectfluent#595 (comment)

Vinnl · 2022-10-27T09:34:55Z

Thanks for getting back on it @eemeli, applied your suggestions!

… Localized

eemeli

I reverted the Children.only() addition and applied a little code styling. Let's finally merge this! 🎉

Vinnl · 2022-10-28T09:36:57Z

Thank you so much @eemeli!

Vinnl · 2022-11-15T15:30:18Z

Hi @eemeli, sorry to bug you again about this, but I was wondering if we could get a release of @fluent/react with this in it? :)

See projectfluent/fluent.js#595 (comment)

Add getFragment method to ReactLocalization

04bb1a0

It's similar to getString(), but returns a React Fragment instead of a string. The main added benefit here is that it allows the caller to pass in `elems` analogous to the similarly-named prop on <Localized>.

Vinnl mentioned this pull request Jul 9, 2022

withLocalization's getString API is missing the elems parameter #544

Closed

Vinnl mentioned this pull request Aug 6, 2022

Add clearer tracker removal breakage warnings mozilla/fx-private-relay#2301

Merged

5 tasks

Vinnl added 2 commits September 1, 2022 17:51

Add getElement, redefine getFragment using it

cdf5ea9

Re-define <Localized> using getElement

695874d

eemeli requested changes Sep 5, 2022

View reviewed changes

Vinnl added 5 commits September 8, 2022 21:30

fixup! Add getElement, redefine getFragment using it

435a08b

Mark all empty ID values as invalid

29be3dd

As per projectfluent#595 (comment)

fixup! Add getElement, redefine getFragment using it

ab17fdb

Use a Map for a record

a9b73ca

As per projectfluent#595 (comment)

Desructure a parameter

87c73d4

As per projectfluent#595 (comment)

Vinnl requested a review from eemeli September 9, 2022 07:04

eemeli reviewed Sep 9, 2022

View reviewed changes

fluent-react/src/localization.ts Outdated Show resolved Hide resolved

eemeli requested changes Sep 9, 2022

View reviewed changes

Vinnl added 6 commits September 12, 2022 15:58

fixup! Use a Map for a record

515918e

Remove getFragment

6027f43

It's a thin wrapper, so not worth it for now to spend additional API surface area on, as per projectfluent#595 (comment)

Rename second getString parameter to vars

f2b728c

It's equivalent to the `vars` prop on <Localiszed> and the respective parameter for getElement, holding Fluent variables.

fixup! Add getElement, redefine getFragment using it

998e786

Filter out invalid elems on the first pass

b950f1c

As per projectfluent#595 (comment)

Only accept single element to render in getElement

59671cc

As per projectfluent#595 (comment)

Vinnl requested a review from eemeli September 12, 2022 15:15

eemeli requested changes Oct 26, 2022

View reviewed changes

fluent-react/src/localized.ts Show resolved Hide resolved

fluent-react/src/localized.ts Outdated Show resolved Hide resolved

fluent-react/src/localization.ts Outdated Show resolved Hide resolved

fluent-react/src/localization.ts Outdated Show resolved Hide resolved

Vinnl added 2 commits October 27, 2022 11:24

Outsource one-element checking to Children.only

71dfc62

As per projectfluent#595 (comment)

Empty-initialise unused parameter

dc130a5

As per projectfluent#595 (comment)

Clarify variable name

dbfbdb5

Vinnl requested a review from eemeli October 27, 2022 09:34

eemeli added 2 commits October 27, 2022 09:06

Apply Prettier code styling

8362f23

Revert React.Children.only use, rename componentToRender -> source in…

dcf65c3

… Localized

eemeli approved these changes Oct 27, 2022

View reviewed changes

eemeli merged commit e99614e into projectfluent:master Oct 27, 2022

Vinnl added a commit to mozilla/fx-private-relay that referenced this pull request Apr 3, 2023

Add getFragment method to @fluent/react

450256c

See projectfluent/fluent.js#595 (comment)

Vinnl added a commit to mozilla/fx-private-relay that referenced this pull request Apr 3, 2023

Add getFragment method to @fluent/react

0d5c028

See projectfluent/fluent.js#595 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add getFragment method to ReactLocalization #595

Add getFragment method to ReactLocalization #595

Vinnl commented Jul 9, 2022 •

edited

eemeli commented Aug 3, 2022

Vinnl commented Aug 3, 2022

eemeli commented Aug 3, 2022

Vinnl commented Sep 1, 2022 •

edited

eemeli left a comment

eemeli Sep 5, 2022

Vinnl Sep 8, 2022

eemeli Sep 9, 2022

Vinnl Sep 12, 2022

eemeli Sep 5, 2022

Vinnl Sep 8, 2022

eemeli Sep 9, 2022

Vinnl Sep 12, 2022

eemeli Sep 9, 2022

Vinnl Sep 12, 2022

eemeli left a comment

Vinnl commented Oct 20, 2022

eemeli left a comment

Vinnl commented Oct 27, 2022

eemeli left a comment

Vinnl commented Oct 28, 2022

Vinnl commented Nov 15, 2022

	"Expected to receive a single React node element to inject a localized string into."
	"Expected to receive a single React element to localize"

Add getFragment method to ReactLocalization #595

Add getFragment method to ReactLocalization #595

Conversation

Vinnl commented Jul 9, 2022 • edited

eemeli commented Aug 3, 2022

Vinnl commented Aug 3, 2022

eemeli commented Aug 3, 2022

Vinnl commented Sep 1, 2022 • edited

eemeli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eemeli left a comment

Choose a reason for hiding this comment

Vinnl commented Oct 20, 2022

eemeli left a comment

Choose a reason for hiding this comment

Vinnl commented Oct 27, 2022

eemeli left a comment

Choose a reason for hiding this comment

Vinnl commented Oct 28, 2022

Vinnl commented Nov 15, 2022

Vinnl commented Jul 9, 2022 •

edited

Vinnl commented Sep 1, 2022 •

edited