Fixed an issue with nested contexts unwinding when server rendering. Issue #12984 #12985

ericsoderberghp · 2018-06-05T22:33:48Z

The key change her is to unwind to the previous provider of the same type as the provider being popped.

Addresses #12984.

gaearon · 2018-06-05T23:08:08Z

packages/react-dom/src/server/ReactPartialRenderer.js

+    // find the correct previous provider based on type
+    let previousProvider;
+    if (this.providerIndex > -1) {
+      for (let i = 0; i <= this.providerIndex; i += 1) {


Instead of searching from the bottom, shouldn't we start search at the top and stop at the first match? Try adding more nesting to the test.

Duh. Fixed.

gaearon · 2018-06-05T23:08:36Z

packages/react-dom/src/server/ReactPartialRenderer.js

+    let previousProvider;
+    if (this.providerIndex > -1) {
+      for (let i = 0; i <= this.providerIndex; i += 1) {
+        if (this.providerStack[i] &&


Please be explicit in type checks (e.g. !== null if that's what you mean). Don't rely on falsiness.

OK. I wasn't sure if there was a more Flow appropriate way to check. Flow wanted explicit checks for both null and undefined.

gaearon · 2018-06-05T23:17:04Z

packages/react-dom/src/server/ReactPartialRenderer.js

@@ -692,8 +692,8 @@ class ReactDOMServerRenderer {
    // find the correct previous provider based on type
    let previousProvider;
    if (this.providerIndex > -1) {
-      for (let i = 0; i <= this.providerIndex; i += 1) {
-        if (this.providerStack[i] &&
+      for (let i = this.providerIndex; i >= 0; i -= 1) {


If you don't mind, i-- would feel a bit more intuitive to me

Done. I'm used to not allowing infix from our projects lint rules, old habits.

gaearon · 2018-06-05T23:17:50Z

packages/react-dom/src/server/ReactPartialRenderer.js

-      for (let i = 0; i <= this.providerIndex; i += 1) {
-        if (this.providerStack[i] &&
+      for (let i = this.providerIndex; i >= 0; i -= 1) {
+        if (this.providerStack[i] !== null && this.providerStack[i] !== undefined &&


So, do we actually need those checks? I wouldn't expect a provider "below" to ever be null. We only clear them out as we pop. As long as we check i doesn't get below zero we should be fine.

Agreed. But, Flow doesn't seem to be aware of that. Perhaps we need some special Flow syntax I'm not aware of, which is highly likely?

I addressed this by changing the array definition to be stricter and pop via setting the length. It didn't seem right to allow null as an array item but then not test for null when iterating over the array, even though the logic of how the array was being manipulated was OK.

gaearon · 2018-06-05T23:18:47Z

packages/react-dom/src/server/ReactPartialRenderer.js

@@ -692,8 +692,8 @@ class ReactDOMServerRenderer {
    // find the correct previous provider based on type
    let previousProvider;
    if (this.providerIndex > -1) {


Seems like we can remove this condition? For loop already covers it.

gaearon · 2018-06-05T23:19:31Z

packages/react-dom/src/server/ReactPartialRenderer.js

+        }
+      }
+    }
+    if (previousProvider) {


Let's initialize it to null and then compare to null instead of a truthy check.

gaearon · 2018-06-05T23:21:19Z

packages/react-dom/src/server/ReactPartialRenderer.js

+    if (this.providerIndex > -1) {
+      for (let i = this.providerIndex; i >= 0; i--) {
+        if (this.providerStack[i] !== null && this.providerStack[i] !== undefined &&
+          (this.providerStack[i]: ReactProvider<any>).type === provider.type) {


This comment was about why we felt comfortable using Flow any:

// We assume this type is correct because of the index check above.

Please keep it above the any case. Maybe change to "Flow type" to disambiguate.

// We assume this Flow type is correct because of the index check above.

gaearon · 2018-06-05T23:21:49Z

GH collapsed some older comments but they're still relevant. Thanks!

aweary · 2018-06-05T23:38:19Z

AFAICT The client implementation uses a stack of cursors for popping/pushing context providers without iteration. Could that same approach be reused here?

gaearon · 2018-06-06T00:16:42Z

AFAICT The client implementation uses a stack of cursors for popping/pushing context providers without iteration. Could that same approach be reused here?

We have many kinds of contexts there (even for a single provider) but here we have just one so I thought normally you probably wouldn't need to go too high up.

So we could consider this but I'd prefer to separate it from the bugfix.

gaearon · 2018-06-06T00:21:08Z

packages/react-dom/src/server/ReactPartialRenderer.js

@@ -686,17 +686,22 @@ class ReactDOMServerRenderer {
        'Unexpected pop.',
      );
    }
-    this.providerStack[this.providerIndex] = null;


What does .length assignment do in V8? I'd like to make sure it doesn't think the array length change is likely to "stay" because it changes all the time. I think just using null is more straightforward and less surprising to the engine (even though the typing isn't as nice for us).

OK. But, going back to using null seems to require testing for null when accessing array items by index, since Flow has been told that null and undefined are expected array item values. Or, is there an alternate Flow syntax that allows us to remove the explicit checks?

gaearon · 2018-06-06T00:21:51Z

packages/react-dom/src/server/ReactPartialRenderer.js

+    let previousProvider = null;
+    for (let i = this.providerIndex; i >= 0; i--) {
+      // We assume this Flow type is correct because of the index check above.
+      if ((this.providerStack[i]: ReactProvider<any>).type === provider.type) {


Please read this.providerStack[i] once. (Move it to a constant.)

gaearon · 2018-06-06T00:22:06Z

packages/react-dom/src/server/ReactPartialRenderer.js

-      const previousProvider: ReactProvider<any> = (this.providerStack[
-        this.providerIndex
-      ]: any);
+    // find the correct previous provider based on type


This comment is redundant, IMO the code speaks for itself

aweary · 2018-06-06T00:29:23Z

We have many kinds of contexts there (even for a single provider) but here we have just one so I thought normally you probably wouldn't need to go too high up.

What do you mean by just having one here? You can use any number of context instances right? For example:

const Foo = React.createContext("foo");
const Bar = React.createContext("bar");

const App = () => (
  <Foo.Provider value="new foo">
    <Bar.Provider>
      <Bar.Provider>
        {/* Imagine this is nested within a bunch of Bar.Providers */}
        <Foo.Provider>{children}</Foo.Provider>
      </Bar.Provider>
    </Bar.Provider>
  </Foo.Provider>
);

When the deeply nested Foo.Provider gets popped the provider stack would have a bunch of Bar providers that it had to iterate through to get to the next Foo provider. That would mean that the runtime of popping a provider is bound by the number of other providers up the tree. Or am I misunderstanding how the provider stack works?

gaearon · 2018-06-06T00:54:24Z

I mean one per provider (in Fiber every provider has three context stack values associated with it). And we also keep track of host context and a bunch of other things in that client stack. So there is good justification for over-engineering that client stack.

I agree with what you're saying in general. If it's avoidable let's avoid it.

… values

pull-bot · 2018-06-06T01:07:24Z

Details of bundled changes.

Comparing: c5a733e...5114a3e

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom-server.browser.development.js	+0.3%	+0.4%	101.54 KB	101.88 KB	26.48 KB	26.58 KB	UMD_DEV
react-dom-server.browser.production.min.js	🔺+0.4%	🔺+0.6%	15.06 KB	15.11 KB	5.76 KB	5.79 KB	UMD_PROD
react-dom-server.browser.development.js	+0.4%	+0.4%	90.84 KB	91.18 KB	24.23 KB	24.32 KB	NODE_DEV
react-dom-server.browser.production.min.js	🔺+0.4%	🔺+0.4%	14.42 KB	14.48 KB	5.51 KB	5.54 KB	NODE_PROD
react-dom-server.node.development.js	+0.4%	+0.4%	92.77 KB	93.1 KB	24.77 KB	24.87 KB	NODE_DEV
react-dom-server.node.production.min.js	🔺+0.4%	🔺+0.5%	15.23 KB	15.28 KB	5.81 KB	5.83 KB	NODE_PROD
ReactDOMServer-dev.js	+0.4%	+0.4%	94.62 KB	94.96 KB	24.1 KB	24.19 KB	FB_WWW_DEV
ReactDOMServer-prod.js	🔺+0.9%	🔺+0.8%	31.39 KB	31.68 KB	7.71 KB	7.77 KB	FB_WWW_PROD

Generated by 🚫 dangerJS

gaearon · 2018-06-06T22:09:52Z

packages/react-dom/src/server/ReactPartialRenderer.js

-        priorProvider.type === provider.type) {
+      if (
+        priorProvider !== null &&
+        priorProvider !== undefined &&


These checks aren’t truly necessary, are they? We know they can’t be empty because we’re moving to the left and check the index.

If type system disagrees I think we should do an unsafe cast with a comment.

Happy to. I wasn't sure how strict you wanted to be with the type checking.

SamyPesse · 2018-06-11T06:55:22Z

@ericsoderberghp @gaearon Is there any changes left to make on this PR ? Is there any way I can help ?

It's impacting us at GitBook, I've never contributed to the react core, but I'd be happy to start :)

gaearon · 2018-06-11T12:44:03Z

@SamyPesse

Hi! One thing that concerns me about this solution is that it's O(n) where n is the number of providers on the stack. It would be good to avoid iterating over them as we pop them.

I'll get this fix in but I'll file a followup issue to try to remove the iteration. Do you want to try to help with this? I'm a bit hesitant to cut a release without a fix for this.

aweary · 2018-06-11T16:19:42Z

@gaearon that does make it a lot clearer, thank you! So the key to making this work is pushing the previous Provider into the stack when a Provider is pushed.

                       A    B    stack

                       A0    B0   []
push A1                A1    B0   [A0]
push   B1              A1    B1   [A0, B0]
push     B2            A1    B2   [A0, B0, B1]
push       B3          A1    B3   [A0, B0, B1, B2]
push         A2        A2    B3   [A0, B0, B1, B2, A1]
pop          A2        A1    B1   [A0, B0, B1, B2]
...

gaearon · 2018-06-11T16:20:30Z

Yep.

                      A    B    values               providers

init                 A0    B0   []                   []
push A1              A1    B0   [A0]                 [A]
push   B1            A1    B1   [A0, B0]             [A, B]
push     A2          A2    B1   [A0, B0, A1]         [A, B, A]
push       B2        A2    B2   [A0, B0, A1, B1]     [A, B, A, B]
pop        B2        A2    B1   [A0, B0, A1]         [A, B, A]
pop      A2          A1    B1   [A0, B0]             [A, B]
pop    B1            A1    B0   [A0]                 [A]
pop  A1              A0    B0   []                   []

push(next_provider, next_value):
  index++
  values[index] = next_provider.current_value
  providers[index] = next_provider
  next_provider.current_value = next_value

pop():
  prev_value = values[index]
  prev_provider = providers[index]
  values[index] = null
  providers[index] = null
  index--
  prev_provider.current_value = prev_value

gaearon · 2018-06-11T16:21:00Z

omg did I just accidentally write some Python

ericsoderberghp · 2018-06-11T16:22:00Z

I think what it means is that at the time A2 is encountered, you need a reference to A1 to be able to push it.

gaearon · 2018-06-11T16:24:51Z

Yep, so we keep two stacks. One for "previous values to restore to" and one for "providers who need restoration".

ericsoderberghp · 2018-06-11T16:38:59Z

OK. I'm happy to work on this if you'd like the help.

gaearon · 2018-06-11T16:40:51Z

I have it running so I'll send a PR shortly. Would appreciate a review.

gaearon · 2018-06-11T17:13:28Z

#13019

193: Update dependency react to v16.4.1 r=magopian a=renovate[bot] This Pull Request updates dependency [react](https://github.com/facebook/react) from `v16.4.0` to `v16.4.1` <details> <summary>Release Notes</summary> ### [`v16.4.1`](https://github.com/facebook/react/blob/master/CHANGELOG.md#1641-June-13-2018) [Compare Source](facebook/react@v16.4.0...v16.4.1) ##### React * You can now assign `propTypes` to components returned by `React.ForwardRef`. ([@bvaughn] in [#12911](`facebook/react#12911)) ##### React DOM * Fix a crash when the input `type` changes from some other types to `text`. ([@spirosikmd] in [#12135](`facebook/react#12135)) * Fix a crash in IE11 when restoring focus to an SVG element. ([@ThaddeusJiang] in [#12996](`facebook/react#12996)) * Fix a range input not updating in some cases. ([@Illu] in [#12939](`facebook/react#12939)) * Fix input validation triggering unnecessarily in Firefox. ([@nhunzaker] in [#12925](`facebook/react#12925)) * Fix an incorrect `event.target` value for the `onChange` event in IE9. ([@nhunzaker] in [#12976](`facebook/react#12976)) * Fix a false positive error when returning an empty `<React.Fragment />` from a component. ([@philipp-spiess] in [#12966](`facebook/react#12966)) ##### React DOM Server * Fix an incorrect value being provided by new context API. ([@ericsoderberghp] in [#12985](`facebook/react#12985), [@gaearon] in [#13019](`facebook/react#13019)) ##### React Test Renderer * Allow multiple root children in test renderer traversal API. ([@gaearon] in [#13017](`facebook/react#13017)) * Fix `getDerivedStateFromProps()` in the shallow renderer to not discard the pending state. ([@fatfisz] in [#13030](`facebook/react#13030)) --- </details> --- This PR has been generated by [Renovate Bot](https://renovatebot.com). Co-authored-by: Renovate Bot <bot@renovateapp.com>

25: Update react monorepo to v16.4.1 r=renovate[bot] a=renovate[bot] This Pull Request renovates the package group "react monorepo". - [react-dom](https://github.com/facebook/react) (`dependencies`): from `16.4.0` to `16.4.1` - [react](https://github.com/facebook/react) (`dependencies`): from `16.4.0` to `16.4.1` # Release Notes <details> <summary>facebook/react</summary> ### [`v16.4.1`](https://github.com/facebook/react/blob/master/CHANGELOG.md#1641-June-13-2018) [Compare Source](facebook/react@v16.4.0...v16.4.1) ##### React * You can now assign `propTypes` to components returned by `React.ForwardRef`. ([@bvaughn] in [#12911](`facebook/react#12911)) ##### React DOM * Fix a crash when the input `type` changes from some other types to `text`. ([@spirosikmd] in [#12135](`facebook/react#12135)) * Fix a crash in IE11 when restoring focus to an SVG element. ([@ThaddeusJiang] in [#12996](`facebook/react#12996)) * Fix a range input not updating in some cases. ([@Illu] in [#12939](`facebook/react#12939)) * Fix input validation triggering unnecessarily in Firefox. ([@nhunzaker] in [#12925](`facebook/react#12925)) * Fix an incorrect `event.target` value for the `onChange` event in IE9. ([@nhunzaker] in [#12976](`facebook/react#12976)) * Fix a false positive error when returning an empty `<React.Fragment />` from a component. ([@philipp-spiess] in [#12966](`facebook/react#12966)) ##### React DOM Server * Fix an incorrect value being provided by new context API. ([@ericsoderberghp] in [#12985](`facebook/react#12985), [@gaearon] in [#13019](`facebook/react#13019)) ##### React Test Renderer * Allow multiple root children in test renderer traversal API. ([@gaearon] in [#13017](`facebook/react#13017)) * Fix `getDerivedStateFromProps()` in the shallow renderer to not discard the pending state. ([@fatfisz] in [#13030](`facebook/react#13030)) --- </details> --- This PR has been generated by [Renovate Bot](https://renovatebot.com). Co-authored-by: Renovate Bot <bot@renovateapp.com>

Fixed an issue with nested contexts unwinding when server rendering. …

8ff3f62

…GitHub issue facebook#12984

facebook-github-bot added the CLA Signed label Jun 5, 2018

aweary requested a review from acdlite June 5, 2018 22:45

gaearon reviewed Jun 5, 2018

View reviewed changes

Fixed an issue with search direction and stricter false checking

27bb329

gaearon reviewed Jun 5, 2018

View reviewed changes

Use decrement infix operator

8d38f45

gaearon reviewed Jun 5, 2018

View reviewed changes

Streamlined existence checks

037c4d5

gaearon reviewed Jun 6, 2018

View reviewed changes

Streamlined assignment. Removed redundant comment. Use null for array…

e69904c

… values

Made prettier

731c686

gaearon reviewed Jun 6, 2018

View reviewed changes

Relaxed type checking and improved comment

6d31d3a

aweary mentioned this pull request Jun 7, 2018

Failing test for Context bug in ReactPartialRenderer #12974

Closed

Improve test coverage

5114a3e

gaearon mentioned this pull request Jun 11, 2018

Popping context is O(1) in SSR #13019

Merged

dependencies bot mentioned this pull request Oct 1, 2018

Update react-dom in /modules/frontend from 16.3.0 to 16.5.2 project-accelerate/accelerate#277

Open

This was referenced Nov 1, 2018

Update react-dom in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#253

Closed

Update react in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#254

Closed

This was referenced Dec 1, 2018

Update react-test-renderer in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#312

Open

Update react in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#313

Open

stevenbuccini mentioned this pull request Jan 19, 2019

Throwing an error from a component while server rendering changes the default value of a context if there is a provider in that tree. #14502

Closed

This was referenced Mar 15, 2021

chore(deps): bump react-dom from 16.4.0 to 16.4.2 in /intro-react-v5 samuelsilvadev/front-end-master-courses#9

Merged

Bump react-dom from 16.4.0 to 16.4.2 in /client cdhenry/pivot#1

Open

Fixed an issue with nested contexts unwinding when server rendering. Issue #12984 #12985

Fixed an issue with nested contexts unwinding when server rendering. Issue #12984 #12985

Conversation

ericsoderberghp commented Jun 5, 2018 • edited

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

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

gaearon commented Jun 5, 2018

aweary commented Jun 5, 2018

gaearon commented Jun 6, 2018

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

aweary commented Jun 6, 2018

gaearon commented Jun 6, 2018

pull-bot commented Jun 6, 2018 • edited

react-dom

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamyPesse commented Jun 11, 2018

gaearon commented Jun 11, 2018

aweary commented Jun 11, 2018

gaearon commented Jun 11, 2018

gaearon commented Jun 11, 2018

ericsoderberghp commented Jun 11, 2018

gaearon commented Jun 11, 2018

ericsoderberghp commented Jun 11, 2018

gaearon commented Jun 11, 2018

gaearon commented Jun 11, 2018

ericsoderberghp commented Jun 5, 2018 •

edited

pull-bot commented Jun 6, 2018 •

edited