Use React's new Context API #406

Gregoor · 2019-07-24T16:58:40Z

After talking with Stas it sounded like all of our internal projects are using a recent enough version of React to allow us to switch from the legacy to the new context API. My motivation is to add hooks to fluent. That'd follow in a later PR.

Functionality wise everything should be there for Localized, working at fixing the tests for it next. After that I'll look at withLocalization.

This would also include an API change, namely not exporting ReactLocalization anymore. With hooks it was possible to merge the same functionality into LocalizationProvider without it getting noisy (at least not to me)

Gregoor · 2019-07-31T18:01:44Z

Went with using jest after all. Generation of the snapshot tests was super easy, I've opted for inline mode as that makes it easier to see the expected results (and easier to compare before/after). Renamed the files according to jest's convention as well. Unfortunately that led to git losing track of some 🙈
Another downside is that Jest's snapshot output what the HTML tree looks like and there are some tests in here that check whether Fragment's are retained. Haven't yet figured out how to make those tests meaningful again.

Taking another week-or-so break from this, but if the general Jest direction is alright, I'd continue with that.

cc @stasm

stasm · 2019-07-31T19:31:57Z

Oh, this is great!

Another downside is that Jest's snapshot output what the HTML tree looks like and there are some tests in here that check whether Fragment's are retained. Haven't yet figured out how to make those tests meaningful again.

I added the Fragment tests recently to make sure they can be consumed by Localized. I don't think we need to ensure they're retained after render. I'd be OK with testing the HTML tree instead.

Taking another week-or-so break from this, but if the general Jest direction is alright, I'd continue with that.

I'm 100% for this change. Thank you!

stasm · 2019-07-31T19:33:19Z

Also, sorry about the conflicts your branch has against master now 😔 I hope they're not too hard to resolve.

Gregoor · 2019-08-01T09:28:37Z

Cool! :)

No worries about the conflicts, that's what happens when I create monolithic PRs against an active project 😁

Gregoor · 2019-08-06T18:32:51Z

alrighty, synced it up with master and updated withLocalization (+ its tests) to use the new context API as well.

I'm wondering if the provider_change and provider_context tests are worth updating, but that's related to my ideology being "only write tests for your API, not your internals", and in this case they're testing things that are already covered by the localized tests. If someone disagrees, I can also tackle those in my next session.

Apart from that outstanding todos are:

fix linting issues (ignored those so far)
make sure the project wide test commands run jest
...and I think that'd be it.

Gregoor · 2019-08-07T10:43:09Z

I might need some help on modifiying common.mk. For fluent-react it would just need to run jest --collect-coverage, but my bash scripting skills are not immediately up for the task.

Pike · 2019-08-07T10:47:53Z

I might need some help on modifiying common.mk. For fluent-react it would just need to run jest --collect-coverage, but my bash scripting skills are not immediately up for the task.

I'd look into make and not bash for this, something like https://www.gnu.org/software/make/manual/html_node/Overriding-Makefiles.html sounds interesting. Or just try to overwrite test:.

stasm · 2019-08-08T06:48:34Z

I think make doesn't allow overwriting targets. That's why in common.mk there's a check to decide which version of test to run: with or without test/index.js which sets things up if needed:

fluent.js/common.mk

Lines 22 to 23 in 4312437

    
           test: 
        
           ifneq (,$(wildcard ./test/index.js))

Thinking about it, I probably got ahead of myself here. We only have 8 packages. It shouldn't be a lot of work to just move the test target to each package's Makefile, and it will allow much more flexibility when testing.

stasm · 2019-08-08T06:50:23Z

I'm wondering if the provider_change and provider_context tests are worth updating, but that's related to my ideology being "only write tests for your API, not your internals", and in this case they're testing things that are already covered by the localized tests. If someone disagrees, I can also tackle those in my next session.

I think it's fine to remove them.

Gregoor · 2019-08-12T13:04:16Z

Cool! 👌

I made the makefile change, but I'm not the most experienced makefile-engineer, so maybe someone wants to double-check what I did there.
Edit: Oh I think it was possible to overwrite tasks after all, so should I do that instead of moving tests into all the makefiles?

Had a bit of a change of mind wrt including the hook with this change: If people have to upgrade to a React-hook version to use this anyway, maybe they should be rewarded with a hook. Wdyt? Or should I rewrite this to not use hooks anyway? My thinking is that an upgrade inside of React 16 is painless (no breaking changes), so we might as well skip < 16.8

fluent-react/src/localized.js

dvdmgl · 2019-09-02T10:50:23Z

fluent-react/src/localized.js

+  const { l10n, parseMarkup } = useContext(FluentContext);
+
+  if (l10n === null) {
+    throw new Error(


Maybe log a warning and return child

Throwing an error here changes the current behavior, which is to return the child element as the fallback, as @dvdmgl says. IIRC the purpose of not throwing was to make it easier to test code which uses <Localized> without having to set up a provider. I'm open to revisiting this but I think it would be safer to do it in a separate PR.

Oh, I see another check for !l10n down below in line 70. I'd say this one up here is redundant then.

stasm · 2019-11-25T18:11:49Z

I'm back from my leave of absence, and I'd like to help land this PR. Thanks again to @Gregoor for submitting this PR and to @dvdmgl for the comments.

@Gregoor Would you be available and would like to finish this PR?

Had a bit of a change of mind wrt including the hook with this change: If people have to upgrade to a React-hook version to use this anyway, maybe they should be rewarded with a hook. Wdyt? Or should I rewrite this to not use hooks anyway? My thinking is that an upgrade inside of React 16 is painless (no breaking changes), so we might as well skip < 16.8

I think the use of hooks in this PR is fine. I was wodering if it would make sense to use the Context API verbatim, and even let users wrap their apps in <LocalizationContext.Provider> but if I understand correctly, we still need the custom LocalizationProvider to perform all the initialization of the context value.

When you say rewarded with a hook, do you mean something like a useLocalization hook provided by fluent-react? I'd like to do this for sure, but I'd prefer to do it in another PR to keep the scope of this one small.

Gregoor · 2019-11-26T16:05:26Z

Welcome back, Staś! Good to have you back, hope your time off was as great as it looked on Twitter :)
Happy to continue work on this, I think longer-term MDN will switch to Fluent so I think this is justified.

When you say rewarded with a hook, do you mean something like a useLocalization hook provided by fluent-react? I'd like to do this for sure, but I'd prefer to do it in another PR to keep the scope of this one small.

Huh, from reading my comment it seems that's what I meant but I can't say that I would still be able to argue for it 😁

Gregoor · 2019-12-04T16:36:48Z

@stasm I'm wondering what's left to be done here. From reading the comments, I think this was the only one outstanding from my side:

I made the makefile change, but I'm not the most experienced makefile-engineer, so maybe someone wants to double-check what I did there.
Edit: Oh I think it was possible to overwrite tasks after all, so should I do that instead of moving tests into all the makefiles?

stasm · 2019-12-05T09:55:03Z

Moving the test target into individual packages' Makefiles sounds good to me. In fact, I did the same in #436. I might take a closer look at common.mk in the near future; it's not as helpful as I though it would be.

I'll start reviewing this today, thanks @Gregoor!

stasm

Nice work, this is very close, but I'd like to make a few small changes before merging. My comments are below.

stasm · 2019-12-10T09:59:03Z

fluent-react/src/localized.js

+  const { l10n, parseMarkup } = useContext(FluentContext);
+
+  if (l10n === null) {
+    throw new Error(


Throwing an error here changes the current behavior, which is to return the child element as the fallback, as @dvdmgl says. IIRC the purpose of not throwing was to make it easier to test code which uses <Localized> without having to set up a provider. I'm open to revisiting this but I think it would be safer to do it in a separate PR.

stasm · 2019-12-10T10:00:24Z

fluent-react/src/localized.js

+  const { l10n, parseMarkup } = useContext(FluentContext);
+
+  if (l10n === null) {
+    throw new Error(


Oh, I see another check for !l10n down below in line 70. I'd say this one up here is redundant then.

stasm · 2019-12-10T12:00:20Z

fluent-react/babel.config.js

@@ -0,0 +1,15 @@
+module.exports = {


Is this file required for running tests with Jest? Please add a one-line comment explaining why we need it :)

fluent-react/makefile

stasm · 2019-12-10T12:16:21Z

fluent-react/src/provider.js

  }

-  componentWillReceiveProps(next) {
-    const { bundles } = next;
+  const bundles = useMemo(() => CachedSyncIterable.from(props.bundles), [


I'm concerned about the use of useMemo here. To quote the docs:

You may rely on useMemo as a performance optimization, not as a semantic guarantee. In the future, React may choose to “forget” some previously memoized values and recalculate them on next render, e.g. to free memory for offscreen components. Write your code so that it still works without useMemo — and then add it to optimize performance.

I'd prefer to use an API with stronger guarantees here, e.g. useEffect. Or, as you suggested on Slack, refactor this to memoize the entire context object.

stasm · 2019-12-10T12:21:03Z

fluent-react/src/provider.js

+  ]);
+  const contextValue = useMemo(
+    () => {
+      const l10n = {


I think I preferred it when this was a separate ReactLocalization class in its own file. What do you think? I see that right now you're relying on a closure to have access to bundles; I'd OK with re-creating the instance of ReactLocalization when the props to the provider change.

This would be in line with your proposal from Slack to memoize the entire context rather than individual props, right?

I wanted to (and now did) memoize the whole Provider component and not just the contextValue. That should make things a lot clearer. I also added a test to make sure that it stop unnecessary re-renders. It doesn't have the drawbacks of useMemo, namely

no extra per-render array+function-scope allocations

(probably) no future react might forget your memoization (at least not according to the docs)

A memoized (aka pure) component does an additional thing which a non-memoized Component doesn't do: shallowly compare props. But that's cheap enough and seems worth it, compared to (re)iterating the bundles.

Wrt the ReactLocalization class: I already had a comment typed out protesting against it, but then I gave myself a real think about it and ended up agreeing and bringing it back. Apparently I also removed ReactLocalization's comments in the process without finding a better place for them. Shame on me!
For posterity reasons why I think bringing it back is a good idea:

JS engines do a better job at optimizing for classes with known methods and fields

the concerns of the Provider and the Localization are separate enough

context is initialized with the same type as it's being used with (which I failed to do before)

And for completeness-sake a reason against it would've been that it moves related code further apart, making it potentially a little harder to grok and edit.

Thanks for the thorough review!

stasm · 2019-12-10T12:21:41Z

fluent-react/src/with_localization.js

-        )
-      );
-    }
+  function WithDisplay(props) {


What's the reason for this rename?

I honestly have no clue what happened there. Looks like an accident, I reverted it. Thanks for catching it!

It simplifies the code a lot and might also make it faster, as memoization during render comes at the cost of additional array and function scope allocation. This also adds a test to check that the Provider is not re-rendered when the bundles prop does not change.

Co-Authored-By: Staś Małolepszy <[email protected]>

stasm

Nice, this looks great. I have one more comment and a question, and we're done :)

I'd prefer to not add new Babel plugins, in this case @babel/plugin-proposal-class-properties. I'm on a quest to simplify the build setup across all Fluent packages. I hope to be able to remove @babel/plugin-proposal-async-generator-functions, too, when we drop Node.js 8 support in Require Node.js 10 #341.
I have a question below about what the value of the context should be. Right now it's {l10n, parseMarkup}, but I wouldn't mind moving parseMarkup into ReactLocalization, similar to reportError, and using the instance of ReactLocalization as the context value. What do you think?

fluent-react/src/localization.js

stasm · 2019-12-12T08:19:52Z

fluent-react/src/provider.js

+    FluentContext.Provider,
+    { value: {
+      l10n: new ReactLocalization(props.bundles, props.parseMarkup),
+      parseMarkup: props.parseMarkup || createParseMarkup()


As it stands right now, the ReactLocalization constructor doesn't take parseMarkup as an argument. That said, I wonder if it would actually be a good idea to make the value of the context be the instance of ReactLocalization, rather than the {l10n, parseMarkup} object. Then, passing parseMarkup to the constructor would be justified. @Gregoor, what do you think?

I'm not sure about this one yet, as I don't see a semantic reason for them to live together. Are there other ergonomic/performance considerations that might make us want to do that?

I'm mostly basing this on how this works in @fluent/dom. There we have the DOMLocalization class which has all the logic for localizing DOM elements. I'd lean towards considering parseMarkup part of ReactLocalization, too.

In longer term, I expect parseMakrup to be removed as a configurable property, which would leave l10n as the only property of the context value. I don't see many benefits to keeping this extra level of nesting, but perhaps it's because I generally tend to prefer flatter structures :)

fluent-react/src/context.js

stasm · 2020-01-10T13:51:26Z

I'd like to change the value of the context to be the instance of ReactLocalization, but I'd also like to land this PR :) @Gregoor, any objections to landing it as-is? I'll open a new PR to consider the context value change.

Gregoor · 2020-01-10T14:21:15Z

I'd like to change the value of the context to be the instance of ReactLocalization, but I'd also like to land this PR :) @Gregoor, any objections to landing it as-is? I'll open a new PR to consider the context value change.

I remember typing a reply before the holidays and having some thoughts about this (agreeing with you), but I didn't seem to have send it 🤔 I'm happy either way!

stasm

Alright, thanks. Let's land this as-is, and I'll file a follow-up about the context value.

Gregoor added 2 commits August 2, 2019 14:38

initial version with React's new Context API

6715562

use jest for snapshot testing localized

55db0be

Gregoor force-pushed the new-context-api branch from cf63fd4 to 55db0be Compare August 6, 2019 17:37

use new context API in withLocalization and update its tests

0816b85

fix linting issues

a1d186a

Gregoor added 2 commits August 12, 2019 14:19

Remove tests not covering fluent-react's API

a603ffa

Move test task into each separate makefile

b23bf28

dvdmgl reviewed Sep 2, 2019

View reviewed changes

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

dvdmgl reviewed Sep 2, 2019

View reviewed changes

Pike mentioned this pull request Oct 2, 2019

hoist static functions in withLocalization #430

Open

stasm suggested changes Dec 10, 2019

View reviewed changes

This was referenced Dec 11, 2019

stop using componentWillReceiveProps() (#425) #440

Closed

@fluent/react: stop using componentWillReceiveProps() #425

Closed

Gregoor and others added 3 commits December 11, 2019 18:00

Update fluent-react/makefile

8f618ef

Co-Authored-By: Staś Małolepszy <[email protected]>

Add comment explaining that we need babel.config.js for Jest

a67a0b7

Gregoor added 3 commits December 11, 2019 18:08

Remove error throwing behavior for Localized without a Provider

8021604

Fix erroneous WithLocalization renaming

61a6219

Bring ReactLocalization back to encapsulate bundles and related methods

ea35184

Gregoor changed the title ~~WIP Use React's new Context API~~ Use React's new Context API Dec 11, 2019

stasm suggested changes Dec 12, 2019

View reviewed changes

Remove babel class properties transform

0a83a19

Gregoor force-pushed the new-context-api branch from 8d563cb to 0a83a19 Compare December 13, 2019 11:17

Correctly initialize context

d02a9c5

Gregoor force-pushed the new-context-api branch from 2fe7e40 to d02a9c5 Compare December 17, 2019 13:21

stasm approved these changes Jan 10, 2020

View reviewed changes

stasm merged commit 0bc8a06 into projectfluent:master Jan 10, 2020

stasm mentioned this pull request Apr 6, 2020

Use the new React Context API #182

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use React's new Context API #406

Use React's new Context API #406

Gregoor commented Jul 24, 2019

Gregoor commented Jul 31, 2019

stasm commented Jul 31, 2019

stasm commented Jul 31, 2019

Gregoor commented Aug 1, 2019

Gregoor commented Aug 6, 2019 •

edited

Loading

Gregoor commented Aug 7, 2019

Pike commented Aug 7, 2019

stasm commented Aug 8, 2019

stasm commented Aug 8, 2019

Gregoor commented Aug 12, 2019 •

edited

Loading

dvdmgl Sep 2, 2019

stasm Dec 10, 2019 •

edited

Loading

stasm Dec 10, 2019

stasm commented Nov 25, 2019

Gregoor commented Nov 26, 2019

Gregoor commented Dec 4, 2019

stasm commented Dec 5, 2019

stasm left a comment

stasm Dec 10, 2019 •

edited

Loading

stasm Dec 10, 2019

stasm Dec 10, 2019

stasm Dec 10, 2019

stasm Dec 10, 2019

stasm Dec 10, 2019

Gregoor Dec 11, 2019

stasm Dec 10, 2019

Gregoor Dec 11, 2019

stasm left a comment

stasm Dec 12, 2019

Gregoor Dec 13, 2019

stasm Dec 18, 2019

stasm commented Jan 10, 2020

Gregoor commented Jan 10, 2020 •

edited

Loading

stasm left a comment •

edited

Loading

Use React's new Context API #406

Use React's new Context API #406

Conversation

Gregoor commented Jul 24, 2019

Gregoor commented Jul 31, 2019

stasm commented Jul 31, 2019

stasm commented Jul 31, 2019

Gregoor commented Aug 1, 2019

Gregoor commented Aug 6, 2019 • edited Loading

Gregoor commented Aug 7, 2019

Pike commented Aug 7, 2019

stasm commented Aug 8, 2019

stasm commented Aug 8, 2019

Gregoor commented Aug 12, 2019 • edited Loading

Choose a reason for hiding this comment

stasm Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stasm commented Nov 25, 2019

Gregoor commented Nov 26, 2019

Gregoor commented Dec 4, 2019

stasm commented Dec 5, 2019

stasm left a comment

Choose a reason for hiding this comment

stasm Dec 10, 2019 • edited Loading

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

stasm 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

stasm commented Jan 10, 2020

Gregoor commented Jan 10, 2020 • edited Loading

stasm left a comment • edited Loading

Choose a reason for hiding this comment

Gregoor commented Aug 6, 2019 •

edited

Loading

Gregoor commented Aug 12, 2019 •

edited

Loading

stasm Dec 10, 2019 •

edited

Loading

stasm Dec 10, 2019 •

edited

Loading

Gregoor commented Jan 10, 2020 •

edited

Loading

stasm left a comment •

edited

Loading