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

Add onErrorShell and Make renderToReadableStream a Promise #23247

Merged
merged 1 commit into from
Feb 9, 2022

Conversation

sebmarkbage
Copy link
Collaborator

This adds onErrorShell as a pair to onCompleteShell. It gets called if something errors outside of a Suspense boundary. In that case, we don't have a complete Shell to emit and the user is expected to emit a different stream instead.

This is slightly different from onError being called before onCompleteShell because we could be rendering inside a Suspense boundary before the shell completes which also triggers onError.

This changes the core SSR API for Web Streams to return a Promise of a ReadableStream instead. It also removes the onCompleteShell callback. If the Promise resolves, it's like onCompleteShell. If the Promise rejects, it's like onErrorShell.

The idea is that you get a stream of the shell from the Promise.

Ideally we would Promisify the Node API too because it better reflects what you're supposed to do (pipe after onCompleteShell). However, that would ideally also include using AbortSignals which is a relatively new thing in Node.js. So I stick to a more old-school Node API signature. I'm not sure how valuable it is to Promisify this API if Node eventually supports Web Streams anyway.

The reason I didn't do this before was because I predicted that we'll want to emit preloads earlier than onCompleteShell. However, the plan now is to do that through an out-of-band callback like onPreload, which can then be used to either emit 103 status responses or start emitting the beginning of a HTML document depending what the embedder prefers.

cc @devknoll

@sizebot
Copy link

sizebot commented Feb 8, 2022

Comparing: cd4eb11...b96ae0e

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 = 130.34 kB 130.34 kB = 41.81 kB 41.81 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 135.52 kB 135.52 kB = 43.34 kB 43.34 kB
facebook-www/ReactDOM-prod.classic.js = 431.25 kB 431.25 kB = 79.10 kB 79.10 kB
facebook-www/ReactDOM-prod.modern.js = 421.19 kB 421.19 kB = 77.68 kB 77.68 kB
facebook-www/ReactDOMForked-prod.classic.js = 431.25 kB 431.25 kB = 79.10 kB 79.10 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/react-server/cjs/react-server.production.min.js +0.38% 19.83 kB 19.90 kB +0.27% 6.91 kB 6.93 kB
oss-stable-semver/react-server/cjs/react-server.production.min.js +0.38% 19.65 kB 19.72 kB +0.29% 6.85 kB 6.87 kB
oss-stable/react-server/cjs/react-server.production.min.js +0.38% 19.65 kB 19.72 kB +0.29% 6.85 kB 6.87 kB
oss-stable-semver/react-dom/umd/react-dom-server.browser.production.min.js +0.28% 32.87 kB 32.97 kB +0.33% 11.30 kB 11.34 kB
oss-stable/react-dom/umd/react-dom-server.browser.production.min.js +0.28% 32.87 kB 32.97 kB +0.33% 11.30 kB 11.34 kB
oss-experimental/react-dom/umd/react-dom-server.browser.production.min.js +0.28% 33.12 kB 33.22 kB +0.36% 11.40 kB 11.44 kB
oss-stable-semver/react-dom/cjs/react-dom-server.browser.production.min.js +0.28% 32.71 kB 32.80 kB +0.34% 11.18 kB 11.22 kB
oss-stable/react-dom/cjs/react-dom-server.browser.production.min.js +0.28% 32.71 kB 32.80 kB +0.34% 11.18 kB 11.22 kB
oss-experimental/react-dom/cjs/react-dom-server.browser.production.min.js +0.28% 32.96 kB 33.05 kB +0.35% 11.27 kB 11.31 kB
oss-stable-semver/react-dom/cjs/react-dom-server.node.production.min.js +0.28% 35.89 kB 35.99 kB +0.22% 12.16 kB 12.18 kB
oss-stable/react-dom/cjs/react-dom-server.node.production.min.js +0.28% 35.89 kB 35.99 kB +0.22% 12.16 kB 12.18 kB
oss-experimental/react-dom/cjs/react-dom-server.node.production.min.js +0.27% 36.20 kB 36.30 kB +0.22% 12.27 kB 12.30 kB
facebook-www/ReactDOMServer-prod.modern.js +0.26% 73.83 kB 74.03 kB +0.25% 15.45 kB 15.49 kB
oss-stable-semver/react-dom/cjs/react-dom-server-legacy.browser.production.min.js +0.23% 32.05 kB 32.13 kB +0.21% 10.75 kB 10.77 kB
oss-stable/react-dom/cjs/react-dom-server-legacy.browser.production.min.js +0.23% 32.05 kB 32.13 kB +0.21% 10.75 kB 10.77 kB
oss-experimental/react-dom/umd/react-dom-server-legacy.browser.production.min.js +0.23% 32.49 kB 32.56 kB +0.18% 10.99 kB 11.01 kB
oss-stable-semver/react-dom/umd/react-dom-server-legacy.browser.production.min.js +0.23% 32.24 kB 32.31 kB +0.17% 10.90 kB 10.92 kB
oss-stable/react-dom/umd/react-dom-server-legacy.browser.production.min.js +0.23% 32.24 kB 32.31 kB +0.17% 10.90 kB 10.92 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.browser.production.min.js +0.23% 32.30 kB 32.38 kB +0.19% 10.84 kB 10.86 kB
oss-stable-semver/react-dom/cjs/react-dom-server-legacy.node.production.min.js +0.21% 35.74 kB 35.81 kB +0.21% 11.99 kB 12.01 kB
oss-stable/react-dom/cjs/react-dom-server-legacy.node.production.min.js +0.21% 35.74 kB 35.81 kB +0.21% 11.99 kB 12.01 kB
oss-stable-semver/react-server/cjs/react-server.development.js +0.21% 124.11 kB 124.37 kB +0.21% 31.17 kB 31.23 kB
oss-stable/react-server/cjs/react-server.development.js +0.21% 124.11 kB 124.37 kB +0.21% 31.17 kB 31.23 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.node.production.min.js +0.21% 36.05 kB 36.12 kB +0.21% 12.09 kB 12.12 kB
oss-experimental/react-server/cjs/react-server.development.js +0.21% 124.63 kB 124.89 kB +0.21% 31.30 kB 31.37 kB

Generated by 🚫 dangerJS against b96ae0e

This indicates that an error has happened before the shell completed and
there's no point in emitting the result of this stream.

This is not quite the same as other fatal errors that can happen even
after streaming as started.

It's also not quite the same as onError before onCompleteShell because
onError can be called for an error inside a Suspense boundary before the
shell completes.

Implement shell error handling in Node SSR fixtures

Instead of hanging indefinitely.

Update Browser Fixture

Expose onErrorShell to the Node build

This API is not Promisified so it's just a separate callback instead.

Promisify the Browser Fizz API

It's now a Promise of a readable stream. The Promise resolves when the
shell completes. If the shell errors, the Promise is rejected.
@sebmarkbage sebmarkbage merged commit 5690932 into facebook:main Feb 9, 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
This indicates that an error has happened before the shell completed and
there's no point in emitting the result of this stream.

This is not quite the same as other fatal errors that can happen even
after streaming as started.

It's also not quite the same as onError before onCompleteShell because
onError can be called for an error inside a Suspense boundary before the
shell completes.

Implement shell error handling in Node SSR fixtures

Instead of hanging indefinitely.

Update Browser Fixture

Expose onErrorShell to the Node build

This API is not Promisified so it's just a separate callback instead.

Promisify the Browser Fizz API

It's now a Promise of a readable stream. The Promise resolves when the
shell completes. If the shell errors, the Promise is rejected.
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.

4 participants