Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix context providers in SSR when handling multiple requests #23171

Merged
merged 3 commits into from
Jan 24, 2022

Conversation

frandiox
Copy link
Contributor

Closes #23089

@sebmarkbage @gaearon Thanks for the pointers. This seems to fix the issue but I'm not very well versed in the fizz code. Please have a look to see if this makes sense.

Summary

Context providers are broken in SSR when handling multiple requests. See #23089 for more information.

How did you test this change?

Failing test submitted by @zythum in #23129 passes with this change. I've also tested a local issue we had in Hydrogen and it seems to be fixed.

@frandiox
Copy link
Contributor Author

CI failed in Download artifacts for base revision 👀

@eps1lon
Copy link
Collaborator

eps1lon commented Jan 24, 2022

CI failed in Download artifacts for base revision 👀

I'd rebase and then push an empty commit to see if this was just flaky CI or a reproducable issue.

@gaearon
Copy link
Collaborator

gaearon commented Jan 24, 2022

Can you cherry-pick the failing test commit and add it to the PR please?

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Jan 24, 2022

This looks like the right fix. We just need the previously failing test pulled in so we don't regress.

To spell it out what's going on here, when we pop all the way to the root we need to push (update current value) all the contexts of the "next" stack. We miss one because it was only in the branches when we didn't quite reach the root.

@sebmarkbage
Copy link
Collaborator

Another test that should also be able to surface this same issue is to have two context without a shared provider but in the same request. Bonus points if you add a previously failing test for that too.

<div>
  <ContextA.Provider value={...}>
    <SuspendHere />
  </ContextA.Provider>
  <ContextB.Provider value={...}>
    ...
  </ContextB.Provider>
</div>

@gaearon
Copy link
Collaborator

gaearon commented Jan 24, 2022

I picked the failing test from #23129. Not sure why it's not showing @zythum's avatar but I left the original author on the commit as is.

@sizebot
Copy link

sizebot commented Jan 24, 2022

Comparing: e28a0db...ed36f94

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 129.59 kB 129.59 kB = 41.55 kB 41.55 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 134.77 kB 134.77 kB = 43.10 kB 43.10 kB
facebook-www/ReactDOM-prod.classic.js = 428.19 kB 428.19 kB = 78.60 kB 78.60 kB
facebook-www/ReactDOM-prod.modern.js = 418.18 kB 418.18 kB = 77.17 kB 77.17 kB
facebook-www/ReactDOMForked-prod.classic.js = 428.19 kB 428.19 kB = 78.60 kB 78.60 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against ed36f94

@gaearon
Copy link
Collaborator

gaearon commented Jan 24, 2022

Another test that should also be able to surface this same issue is to have two context without a shared provider but in the same request

I tried it and it didn't fail with old code:

  // @gate experimental
  fit('should be able to pop context after suspending', async () => {
    class DelayClient {
      get() {
        if (this.resolved) return this.resolved;
        if (this.pending) return this.pending;
        return (this.pending = new Promise(resolve => {
          setTimeout(() => {
            delete this.pending;
            this.resolved = 'OK';
            resolve();
          }, 500);
        }));
      }
    }

    const DelayContext = React.createContext(undefined);
    const Component = () => {
      const client = React.useContext(DelayContext);
      if (!client) {
        return 'context not found.';
      }
      const result = client.get();
      if (typeof result === 'string') {
        return result;
      }
      throw result;
    };

    const OtherContext = React.createContext(undefined);
    const OtherComponent = () => {
      let res = React.useContext(OtherContext);
      if (res !== 'foo') {
        throw Error('no')
      }
      return res
    }

    const client = new DelayClient();
    const {writable, output, completed} = getTestWritable();
    ReactDOMFizzServer.renderToPipeableStream(
      <>
        <DelayContext.Provider value={client}>
          <Suspense fallback="loading">
            <Component />
          </Suspense>
        </DelayContext.Provider>
        <OtherContext.Provider value="foo">
          <OtherComponent />
        </OtherContext.Provider>
      </>
    ).pipe(writable);

    jest.runAllTimers();

    expect(output.error).toBe(undefined);
    expect(output.result).toContain('loading');
    expect(output.result).toContain('foo');

    await completed;

    expect(output.error).toBe(undefined);
    expect(output.result).not.toContain('context never found');
    expect(output.result).toContain('foo');
    expect(output.result).toContain('OK');
  })

What's missing?

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Jan 24, 2022

@gaearon The first time it renders the inner Suspense boundary, it'll synchronously pop back out to the root and try to render its sibling. Try making the sibling also suspend.

You want the first sibling to render its "task" separately from the rest of the tree so that it leaves it hanging. This happens after it retries. So the error can only be observed after it has retried once. If the second one retries on the same ping, it'll probably be observed then.

@gaearon
Copy link
Collaborator

gaearon commented Jan 24, 2022

ok that worked. doesn't need to be different context, just separate providers.

@gaearon gaearon changed the title Fix context providers in SSR when handling multiple requests. Fix context providers in SSR when handling multiple requests Jan 24, 2022
@gaearon gaearon merged commit 529dc3c into facebook:main Jan 24, 2022
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Feb 9, 2022
Summary:
This sync includes the following changes:
- **[a3bde7974](facebook/react@a3bde7974 )**: Exclude react-dom/unstable_testing entry point from stable releases ([#23258](facebook/react#23258)) //<Sebastian Markbåge>//
- **[569093276](facebook/react@569093276 )**: Add onErrorShell Callback ([#23247](facebook/react#23247)) //<Sebastian Markbåge>//
- **[0dedfcc68](facebook/react@0dedfcc68 )**: Update the exports field ([#23257](facebook/react#23257)) //<Sebastian Markbåge>//
- **[9d4e8e84f](facebook/react@9d4e8e84f )**: React Native raw event EventEmitter - intended for app-specific perf listeners and debugging ([#23232](facebook/react#23232)) //<Joshua Gross>//
- **[1dece5235](facebook/react@1dece5235 )**: Add back warning with component stack on Hydration mismatch ([#23241](facebook/react#23241)) //<salazarm>//
- **[cd4eb116c](facebook/react@cd4eb116c )**: Revert "update node.js version for CI ([#23236](facebook/react#23236))" ([#23239](facebook/react#23239)) //<dan>//
- **[1d7728bf9](facebook/react@1d7728bf9 )**: update node.js version for CI ([#23236](facebook/react#23236)) //<sunderls>//
- **[848e802d2](facebook/react@848e802d2 )**: Add onRecoverableError option to hydrateRoot, createRoot ([#23207](facebook/react#23207)) //<Andrew Clark>//
- **[5318971f5](facebook/react@5318971f5 )**: Remove logic for multiple error recovery attempts ([#23227](facebook/react#23227)) //<Andrew Clark>//
- **[3a4462129](facebook/react@3a4462129 )**: Disable avoidThisFallback support in Fizz  ([#23224](facebook/react#23224)) //<salazarm>//
- **[0318ac2c4](facebook/react@0318ac2c4 )**: Revert 4f5449 //<Ricky>//
- **[4f5449eb4](facebook/react@4f5449eb4 )**: Remove main from scheduler `index.js` //<Ricky>//
- **[3f5ff16c1](facebook/react@3f5ff16c1 )**: [Hydration] Fallback to client render if server rendered extra nodes ([#23176](facebook/react#23176)) //<salazarm>//
- **[529dc3ce8](facebook/react@529dc3ce8 )**: Fix context providers in SSR when handling multiple requests ([#23171](facebook/react#23171)) //<Fran Dios>//
- **[505c15c9e](facebook/react@505c15c9e )**: Don't inject timeline hooks unless React supports profiling ([#23151](facebook/react#23151)) //<Brian Vaughn>//
- **[e12a9dfc9](facebook/react@e12a9dfc9 )**: Fix production-only updateSyncExternalStore() crash when doing setState in render ([#23150](facebook/react#23150)) //<Douglas Armstrong>//
- **[e48940255](facebook/react@e48940255 )**: Warn when a callback ref returns a function ([#23145](facebook/react#23145)) //<Dan Abramov>//
- **[d8cfeaf22](facebook/react@d8cfeaf22 )**: Fix context propagation for offscreen/fallback trees ([#23095](facebook/react#23095)) //<Dan Abramov>//
- **[d50482478](facebook/react@d50482478 )**: Enable scheduling profiler flag in react-dom/testing builds ([#23142](facebook/react#23142)) //<Brian Vaughn>//
- **[790b5246f](facebook/react@790b5246f )**: Fix setState ignored in Safari when iframe is added to DOM in the same commit ([#23111](facebook/react#23111)) //<Dan Abramov>//

Changelog:
[General][Changed] - React Native sync for revisions 51947a1...a3bde79

jest_e2e[run_all_tests]

Reviewed By: ShikaSD

Differential Revision: D34108924

fbshipit-source-id: 96acb66c1a7da79d6ef9403490cd0e29ad23d9fb
@gaearon gaearon mentioned this pull request Mar 29, 2022
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
…k#23171)

* add failing test for renderToPipeableStream

* Fix context providers in SSR when handling multiple requests. Closes facebook#23089

* Add sibling regression test

Co-authored-by: zhuyi01 <[email protected]>
Co-authored-by: Dan Abramov <[email protected]>
nevilm-lt pushed a commit to nevilm-lt/react that referenced this pull request Apr 22, 2022
…k#23171)

* add failing test for renderToPipeableStream

* Fix context providers in SSR when handling multiple requests. Closes facebook#23089

* Add sibling regression test

Co-authored-by: zhuyi01 <[email protected]>
Co-authored-by: Dan Abramov <[email protected]>
shwanton pushed a commit to shwanton/react-native-macos that referenced this pull request Feb 13, 2023
Summary:
This sync includes the following changes:
- **[a3bde7974](facebook/react@a3bde7974 )**: Exclude react-dom/unstable_testing entry point from stable releases ([facebook#23258](facebook/react#23258)) //<Sebastian Markbåge>//
- **[569093276](facebook/react@569093276 )**: Add onErrorShell Callback ([facebook#23247](facebook/react#23247)) //<Sebastian Markbåge>//
- **[0dedfcc68](facebook/react@0dedfcc68 )**: Update the exports field ([facebook#23257](facebook/react#23257)) //<Sebastian Markbåge>//
- **[9d4e8e84f](facebook/react@9d4e8e84f )**: React Native raw event EventEmitter - intended for app-specific perf listeners and debugging ([facebook#23232](facebook/react#23232)) //<Joshua Gross>//
- **[1dece5235](facebook/react@1dece5235 )**: Add back warning with component stack on Hydration mismatch ([facebook#23241](facebook/react#23241)) //<salazarm>//
- **[cd4eb116c](facebook/react@cd4eb116c )**: Revert "update node.js version for CI ([facebook#23236](facebook/react#23236))" ([facebook#23239](facebook/react#23239)) //<dan>//
- **[1d7728bf9](facebook/react@1d7728bf9 )**: update node.js version for CI ([facebook#23236](facebook/react#23236)) //<sunderls>//
- **[848e802d2](facebook/react@848e802d2 )**: Add onRecoverableError option to hydrateRoot, createRoot ([facebook#23207](facebook/react#23207)) //<Andrew Clark>//
- **[5318971f5](facebook/react@5318971f5 )**: Remove logic for multiple error recovery attempts ([facebook#23227](facebook/react#23227)) //<Andrew Clark>//
- **[3a4462129](facebook/react@3a4462129 )**: Disable avoidThisFallback support in Fizz  ([facebook#23224](facebook/react#23224)) //<salazarm>//
- **[0318ac2c4](facebook/react@0318ac2c4 )**: Revert 4f5449 //<Ricky>//
- **[4f5449eb4](facebook/react@4f5449eb4 )**: Remove main from scheduler `index.js` //<Ricky>//
- **[3f5ff16c1](facebook/react@3f5ff16c1 )**: [Hydration] Fallback to client render if server rendered extra nodes ([facebook#23176](facebook/react#23176)) //<salazarm>//
- **[529dc3ce8](facebook/react@529dc3ce8 )**: Fix context providers in SSR when handling multiple requests ([facebook#23171](facebook/react#23171)) //<Fran Dios>//
- **[505c15c9e](facebook/react@505c15c9e )**: Don't inject timeline hooks unless React supports profiling ([facebook#23151](facebook/react#23151)) //<Brian Vaughn>//
- **[e12a9dfc9](facebook/react@e12a9dfc9 )**: Fix production-only updateSyncExternalStore() crash when doing setState in render ([facebook#23150](facebook/react#23150)) //<Douglas Armstrong>//
- **[e48940255](facebook/react@e48940255 )**: Warn when a callback ref returns a function ([facebook#23145](facebook/react#23145)) //<Dan Abramov>//
- **[d8cfeaf22](facebook/react@d8cfeaf22 )**: Fix context propagation for offscreen/fallback trees ([facebook#23095](facebook/react#23095)) //<Dan Abramov>//
- **[d50482478](facebook/react@d50482478 )**: Enable scheduling profiler flag in react-dom/testing builds ([facebook#23142](facebook/react#23142)) //<Brian Vaughn>//
- **[790b5246f](facebook/react@790b5246f )**: Fix setState ignored in Safari when iframe is added to DOM in the same commit ([facebook#23111](facebook/react#23111)) //<Dan Abramov>//

Changelog:
[General][Changed] - React Native sync for revisions 51947a1...a3bde79

jest_e2e[run_all_tests]

Reviewed By: ShikaSD

Differential Revision: D34108924

fbshipit-source-id: 96acb66c1a7da79d6ef9403490cd0e29ad23d9fb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

React 18: Context providers are reset to initial value in SSR during rendering
6 participants