Migrate @fluent/react to TypeScript #458

stasm · 2020-03-04T18:11:40Z

See #376 for the general discussion about migrating fluent.js packages to TypeScript.

The migration of @fluent/react is more involved than the packages which I've migrated so far. I'll keep this PR in the draft state for the next couple of days because I'd like to test it in a TypeScript+React project.

This PR changes the API of the <Localized> component. To be fair, I should have made it this way from the get go. I regret giving in to the temptation of the "cute" API design in which arguments to the translations are passed in as props prefixed with the dollar sign: <Localized $arg={...}>. In this PR, the API changes to a more explicit <Localized vars={{arg: ...}}>.

The same goes for elements which are then matched with markup from the translation. Rather than passing them as separate props: <Localized confirmButton={...}>, this PR changes the API to be explicit again: <Localized elems={{confirmButton: ...}}>.

I'm not sure if I got all the ReactNode vs. ReactElement vs. ComponentType right. I'll need to test more and I'd appreciate help with testing!

stasm · 2020-03-05T14:55:04Z

CC @huy-nguyen, @Seally, @Gregoor -- Would you be able to give me feedback on this PR? Thanks!

React typings (wrongly) expect the return type of functional components to be a ReactElement, thus making () => "Hello" invalid. This is incorrect; as of React 16, it's possible to return plain strings from functional components and Component.render(). See DefinitelyTyped#18912 and TypeScript#21699. To work around this, annotate the return value of Localized to be of type ReactElement, and whenever a string or null are returned, wrap them in Fragments.

stasm · 2020-03-06T17:27:45Z

I had to work around the issue described in DefinitelyTyped/DefinitelyTyped#18912. React's typings currently expect the return type of Localized to be a ReactElement rather than ReactNode, which makes TS complain when we try to return null or strings from it (which is valid in React). The work-around is to return React.Fragments instead.

stasm · 2020-03-06T17:28:41Z

I tested this in a few example projects. I think it's ready for review. I'd like to test it in a larger project, if possible, but I don't have good candidates yet.

Gregoor

Two general things that are on my mind:

migration path: This does require all consumers of @fluent/react to change a lot (most?) of their Localized components, doesn't it? So this is a major version change? Is there room for allowing the old way and just throwing warnings for now, so that consumers can migrate gradually?
API / naming: I'm torn on this one. I like brevity but dislike abbreviations. You probably already made the right compromise here and it's not like I have a better one to offer. Just wanted to highlight it.

Exciting changes overall 🎉

Gregoor · 2020-03-13T12:32:07Z

fluent-react/src/localized.ts

@@ -51,8 +45,8 @@ function toArguments(props) {
 *  translation is available.  It also makes it easy to grep for strings in the
 *  source code.
 */
-function Localized(props) {
-  const { id, attrs, children: child = null } = props;
+export function Localized(props: LocalizedProps): ReactElement {


Why not loosen the return type to ReactNode instead, which includes ReactElement but also allows child to be returned without a wrapping call.

I did that initially, but I ran into DefinitelyTyped/DefinitelyTyped#18912. See #458 (comment).

Gregoor · 2020-03-13T12:33:48Z

fluent-react/src/localized.ts

+  id: string;
+  attrs?: Record<string, boolean>;
+  children?: ReactNode;
+  vars?: Record<string, FluentArgument>;


Just out of curiosity: why vars instead of args?

Hmm, I'm torn on this one. They're called variables in Fluent Syntax, but they're passed as arguments. In DOM, we use data-l10n-args, too, but that's mostly an artifact of how we did it in Firefox OS. I think vars is a more accurate name here, but args might be a better choice because of the prior work and due to how developers think about them.

@Pike, @flodolo: do you have any feedback about naming here? We're considering vars vs. args, that is:

<Localized id="hello" vars={{userName: "you"}}/>

vs.

<Localized id="hello" args={{userName: "you"}}/>

My main concern would be to be consistent, and I'd rather change the resolvers as we change resolvers anyway. As opposed to changing the syntax.

So, vars is my vote.

One could argue that you pass your variables as args. As in, when you pass them, they're args, when you store and reference them, they're variables, but that's not useful for anything, I think. (But that may be the historical reason why we have both.)

fluent-react/src/localized.ts

stasm · 2020-03-13T14:39:33Z

Thanks for the comments, @Gregoor!

migration path: This does require all consumers of @fluent/react to change a lot (most?) of their Localized components, doesn't it? So this is a major version change? Is there room for allowing the old way and just throwing warnings for now, so that consumers can migrate gradually?

Yes, it's a breaking change. We might want to release it as 1.0, to make that superclear. I'd also like to figure out the migration plan. We might be able to add the following index signature to Localized: [name: string]: unknown and run the old logic sorting props into arguments and elements, and maybe log a deprecation warning. OTOH, when I upgraded the tests to the new API, it wasn't a lot of work either.

API / naming: I'm torn on this one. I like brevity but dislike abbreviations. You probably already made the right compromise here and it's not like I have a better one to offer. Just wanted to highlight it.

I went with elems and vars for consistency with id and attrs. If we go with longer prop names, it might be a good idea to also rename at least attrs to attributes. Also, if we rename vars to args, then we'd end up with arguments which is special in JS, but maybe that's OK, too. Naming...

stasm · 2020-03-20T10:46:19Z

fluent-react/test/localized_overlay.test.js

@@ -850,8 +855,7 @@ foo = test <em>custom markup parser</em>
    function parseMarkup(str) {
      return [
        {
-          TEXT_NODE: 3,
-          nodeType: 3,
+          nodeName: "#text",


I simplified the detection of text nodes relying on the fact that their nodeName is guaranteed by the standard to be #text.

stasm · 2020-03-20T11:09:45Z

I've finished upgrading voice-web to this new version of @fluent/react in common-voice/common-voice#2631. In the process, I discovered and fixed a few typing issues:

The LocalizedProps and WithLocalizationProps interface are now exported.
The MarkupParser type is now exported.
The type of the withLocalization HOC has been fixed to not require the getString prop in the component returned by the decorator.

I think the remaining work is about naming. vars vs. args, and elems vs. elements. As usual, this is the hardest part :)

stasm · 2020-03-25T12:16:46Z

I think the remaining work is about naming. vars vs. args, and elems vs. elements. As usual, this is the hardest part :)

I'd like to go ahead with vars and elems. We already use short prop names for attrs, and I think vars better describe what's being passed into the translation, contrary to args which emphasize the fact of something being passed in (in a way, elems are also arguments to the translation).

stasm · 2020-03-25T12:22:01Z

migration path: This does require all consumers of @fluent/react to change a lot (most?) of their Localized components, doesn't it? So this is a major version change? Is there room for allowing the old way and just throwing warnings for now, so that consumers can migrate gradually?

@Gregoor I'd like to make this a breaking change in the next release of @fluent/react. Looking at my example upgrade of voice-web in common-voice/common-voice#2631, I think that even though there are many changes, they're also all predictable and almost worthy of a search&replace.

The alternative would be to support both APIs for a period of time, but I'm afraid of having to maintain and test both APIs at the same time, plus their combinations.

Pike

I'm not feeling confident in actually reviewing this, sorry.

I've got some raised eyebrows on the changes to vendor. The files don't seem to match what's in the README anymore?

stasm · 2020-03-25T16:49:19Z

I've got some raised eyebrows on the changes to vendor. The files don't seem to match what's in the README anymore?

Good catch, I reverted them to their originals.

stasm added 2 commits March 4, 2020 19:06

Migrate @fluent/react to TypeScript

cf9527e

Fix paths in fluent-gecko

66c2962

stasm mentioned this pull request Mar 5, 2020

Migrate to TypeScript? #376

Closed

7 tasks

stasm marked this pull request as ready for review March 6, 2020 17:27

Add the build make target

75bbdf6

Gregoor reviewed Mar 13, 2020

View reviewed changes

stasm mentioned this pull request Mar 13, 2020

React i18n with Fluent MozillaFoundation/foundation.mozilla.org#4181

Closed

stasm added 4 commits March 16, 2020 10:00

Fix compat.js make target

6ea604f

Export props interfaces

ed9a3ce

Export MarkupParser type

5de8adb

Fix return type of withLocalization

3bf4fb2

stasm force-pushed the ts-react branch from e787b20 to 3bf4fb2 Compare March 19, 2020 08:27

stasm added 2 commits March 19, 2020 14:20

Convert elems keys to lowercase; lookup via Element.localName

44f6115

Use Node.nodeName.toLowerCase() for elems lookup

1d55914

stasm commented Mar 20, 2020

View reviewed changes

stasm mentioned this pull request Mar 20, 2020

Upgrade @fluent/react, @fluent/bundle common-voice/common-voice#2631

Merged

stasm requested a review from Pike March 25, 2020 12:12

Add a comment about bundle.getMessage(id)!

952690b

Pike reviewed Mar 25, 2020

View reviewed changes

Move src/vendor out of src/

0c6693e

stasm merged commit d6580a5 into projectfluent:master Mar 26, 2020

stasm deleted the ts-react branch March 26, 2020 16:19

stasm mentioned this pull request Apr 22, 2020

Rename FluentArgument to FluentVariable #481

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate @fluent/react to TypeScript #458

Migrate @fluent/react to TypeScript #458

stasm commented Mar 4, 2020 •

edited

Loading

stasm commented Mar 5, 2020

stasm commented Mar 6, 2020

stasm commented Mar 6, 2020

Gregoor left a comment

Gregoor Mar 13, 2020

stasm Mar 13, 2020

Gregoor Mar 13, 2020

stasm Mar 13, 2020

stasm Mar 20, 2020

Pike Mar 20, 2020

stasm commented Mar 13, 2020

stasm Mar 20, 2020

stasm commented Mar 20, 2020

stasm commented Mar 25, 2020

stasm commented Mar 25, 2020

Pike left a comment

stasm commented Mar 25, 2020

Migrate @fluent/react to TypeScript #458

Migrate @fluent/react to TypeScript #458

Conversation

stasm commented Mar 4, 2020 • edited Loading

stasm commented Mar 5, 2020

stasm commented Mar 6, 2020

stasm commented Mar 6, 2020

Gregoor left a comment

Choose a reason for hiding this comment

Gregoor Mar 13, 2020

Choose a reason for hiding this comment

stasm Mar 13, 2020

Choose a reason for hiding this comment

Gregoor Mar 13, 2020

Choose a reason for hiding this comment

stasm Mar 13, 2020

Choose a reason for hiding this comment

stasm Mar 20, 2020

Choose a reason for hiding this comment

Pike Mar 20, 2020

Choose a reason for hiding this comment

stasm commented Mar 13, 2020

stasm Mar 20, 2020

Choose a reason for hiding this comment

stasm commented Mar 20, 2020

stasm commented Mar 25, 2020

stasm commented Mar 25, 2020

Pike left a comment

Choose a reason for hiding this comment

stasm commented Mar 25, 2020

stasm commented Mar 4, 2020 •

edited

Loading