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

[Fizz] Expose a method to abort a pending request #21027

Merged
merged 5 commits into from
Mar 18, 2021

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Mar 18, 2021

This introduces a Set of SuspendedWork to the data structures. This is basically like an AbortSignal. In fact, it should probably be coupled with an actual AbortSignal at the same places.

For work inside a fallback, the abortSet is associated with the boundary that it's a fallback for. This lets the boundary cancel the fallback if the boundary completes.

For all other work, the abortSet is associated with the whole request. This lets you cancel this work for the whole request. If a pending boundary is canceled, then its fallback is canceled too.

I expose this method as an explicit separate control for the Node API. Seems more idiomatic and the follow PR will add such a control anyway. For the Browser API, I accept an actual AbortSignal as an option.

This can be used to implement for example timeout semantics.

There's a subtle difference between this API and just closing the stream. It lets the stream continue emitting whatever is still queued to flush. It also emits a signal to the client that these fallbacks should be client rendered instead of waiting for the server. But we really should have some way to recover from a terminated stream if we're able to hydrate.

This allows us to abort work and put everything into client rendered mode
if we don't want to wait for further I/O.

It also allows us to cancel fallbacks if we complete the main content
before the fallback.
Since this API already returns a value, we need to use destructuring to
expose more options.
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 18, 2021
@sizebot
Copy link

sizebot commented Mar 18, 2021

Comparing: 3fb11ee...28c0533

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 = 122.89 kB 122.89 kB = 39.55 kB 39.55 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 129.38 kB 129.38 kB = 41.61 kB 41.61 kB
facebook-www/ReactDOM-prod.classic.js = 409.01 kB 409.01 kB = 75.85 kB 75.85 kB
facebook-www/ReactDOM-prod.modern.js = 397.27 kB 397.27 kB = 73.91 kB 73.91 kB
facebook-www/ReactDOMForked-prod.classic.js = 409.02 kB 409.02 kB = 75.85 kB 75.85 kB
oss-experimental/react-server/cjs/react-server.development.js +14.07% 28.51 kB 32.52 kB +14.99% 7.29 kB 8.39 kB
oss-stable/react-server/cjs/react-server.development.js +14.07% 28.51 kB 32.52 kB +14.99% 7.29 kB 8.39 kB
oss-experimental/react-server/cjs/react-server.production.min.js +10.02% 7.60 kB 8.37 kB +8.08% 2.81 kB 3.04 kB
oss-stable/react-server/cjs/react-server.production.min.js +10.02% 7.60 kB 8.37 kB +8.08% 2.81 kB 3.04 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.browser.development.js +9.88% 42.88 kB 47.11 kB +10.75% 10.81 kB 11.97 kB
oss-experimental/react-dom/umd/react-dom-unstable-fizz.browser.development.js +9.85% 45.17 kB 49.61 kB +10.71% 10.96 kB 12.13 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.node.development.js +9.40% 43.13 kB 47.19 kB +10.27% 10.82 kB 11.93 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.browser.production.min.js +8.78% 9.53 kB 10.36 kB +7.27% 3.62 kB 3.88 kB
oss-experimental/react-dom/umd/react-dom-unstable-fizz.browser.production.min.js +8.65% 9.72 kB 10.56 kB +7.31% 3.70 kB 3.97 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.node.production.min.js +8.19% 9.72 kB 10.52 kB +6.87% 3.65 kB 3.90 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.development.js +14.07% 28.51 kB 32.52 kB +14.99% 7.29 kB 8.39 kB
oss-stable/react-server/cjs/react-server.development.js +14.07% 28.51 kB 32.52 kB +14.99% 7.29 kB 8.39 kB
oss-experimental/react-server/cjs/react-server.production.min.js +10.02% 7.60 kB 8.37 kB +8.08% 2.81 kB 3.04 kB
oss-stable/react-server/cjs/react-server.production.min.js +10.02% 7.60 kB 8.37 kB +8.08% 2.81 kB 3.04 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.browser.development.js +9.88% 42.88 kB 47.11 kB +10.75% 10.81 kB 11.97 kB
oss-experimental/react-dom/umd/react-dom-unstable-fizz.browser.development.js +9.85% 45.17 kB 49.61 kB +10.71% 10.96 kB 12.13 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.node.development.js +9.40% 43.13 kB 47.19 kB +10.27% 10.82 kB 11.93 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.browser.production.min.js +8.78% 9.53 kB 10.36 kB +7.27% 3.62 kB 3.88 kB
oss-experimental/react-dom/umd/react-dom-unstable-fizz.browser.production.min.js +8.65% 9.72 kB 10.56 kB +7.31% 3.70 kB 3.97 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.node.production.min.js +8.19% 9.72 kB 10.52 kB +6.87% 3.65 kB 3.90 kB
oss-experimental/react-noop-renderer/cjs/react-noop-renderer-server.development.js +1.38% 5.06 kB 5.13 kB +0.96% 1.46 kB 1.47 kB
oss-stable/react-noop-renderer/cjs/react-noop-renderer-server.development.js +1.38% 5.06 kB 5.13 kB +0.96% 1.46 kB 1.47 kB
oss-experimental/react-noop-renderer/cjs/react-noop-renderer-server.production.min.js +1.18% 2.45 kB 2.48 kB +1.26% 0.96 kB 0.97 kB
oss-stable/react-noop-renderer/cjs/react-noop-renderer-server.production.min.js +1.18% 2.45 kB 2.48 kB +1.26% 0.96 kB 0.97 kB

Generated by 🚫 dangerJS against 28c0533

@sebmarkbage
Copy link
Collaborator Author

Since Flight has a similar API maybe it should have the same.

Another API design could be to just provide a timeout option. I'm not sure how else you'd use it other than like timeout 0 (renderToString) or some time based abort.

@sebmarkbage
Copy link
Collaborator Author

sebmarkbage commented Mar 18, 2021

It esthetically bothers me that this is the only method that ruins the otherwise clean W3C stream API. Since you don't need the "startWriting" API in the follow up and I don't expect to need any more. At least for what I know about so far.

Maybe a timeout option would be nicer but more limited.

Edit: Hm. Now that I think about it... AbortSignal is the idiomatic way to do this. We can just accept that as an option. I did that instead.

let request;
const stream = new ReadableStream({
if (options && options.signal) {
options.signal.addEventListener('abort', () => abort(request));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't ever need to be cleaned up, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I would assume the owner would drop the signal when they're done but we might as well clean it up to be sure.

@sebmarkbage sebmarkbage merged commit cf485e6 into facebook:master Mar 18, 2021
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Mar 23, 2021
* Track all suspended work while it's still pending

This allows us to abort work and put everything into client rendered mode
if we don't want to wait for further I/O.

It also allows us to cancel fallbacks if we complete the main content
before the fallback.

* Expose abort API to the browser streams

Since this API already returns a value, we need to use destructuring to
expose more options.

* Add a test including the client actually client rendering it

* Use AbortSignal option for W3C streams instead of external control

* Clean up listener after it's used once
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Mar 23, 2021
Summary:
This sync includes the following changes:
- **[6d3ecb70d](facebook/react@6d3ecb70d )**: Remove unstable_changedBits ([#20953](facebook/react#20953)) //<Andrew Clark>//
- **[754e30728](facebook/react@754e30728 )**: Delete immediateQueueCallbackNode ([#20980](facebook/react#20980)) //<Andrew Clark>//
- **[be5a2e231](facebook/react@be5a2e231 )**: Land enableSyncMicrotasks ([#20979](facebook/react#20979)) //<Andrew Clark>//
- **[f6bc9c824](facebook/react@f6bc9c824 )**: [Fizz] Expose maxBoundarySize as an option ([#21029](facebook/react#21029)) //<Sebastian Markbåge>//
- **[154b85213](facebook/react@154b85213 )**: [Fizz] Expose a method to explicitly start writing to a Node stream ([#21028](facebook/react#21028)) //<Sebastian Markbåge>//
- **[cf485e6f6](facebook/react@cf485e6f6 )**: [Fizz] Expose a method to abort a pending request ([#21027](facebook/react#21027)) //<Sebastian Markbåge>//
- **[3fb11eed9](facebook/react@3fb11eed9 )**: React Native: Touch Instrumentation Interface ([#21024](facebook/react#21024)) //<Timothy Yung>//
- **[825c3021f](facebook/react@825c3021f )**: Don't delete trailing mismatches during hydration at the root ([#21021](facebook/react#21021)) //<Sebastian Markbåge>//
- **[1d1e49cfa](facebook/react@1d1e49cfa )**: [Fizz] Assign an ID to the first DOM element in a fallback or insert a dummy (and testing infra) ([#21020](facebook/react#21020)) //<Sebastian Markbåge>//
- **[466b26c92](facebook/react@466b26c92 )**: Store commit durations on HostRoot for DevTools access ([#20983](facebook/react#20983)) //<Brian Vaughn>//
- **[89acfa639](facebook/react@89acfa639 )**: Fix native event batching in concurrent mode ([#21010](facebook/react#21010)) //<Ricky>//
- **[0203b6567](facebook/react@0203b6567 )**: chore: update react-reconciler README ([#21016](facebook/react#21016)) //<susiwen8>//

Changelog:
[General][Changed] - React Native sync for revisions c9f6d0a...6d3ecb7

jest_e2e[run_all_tests]

Reviewed By: JoshuaGross, kacieb

Differential Revision: D27231625

fbshipit-source-id: 89c0c0662e69044ae8890486a693013bee6005dd
koto pushed a commit to koto/react that referenced this pull request Jun 15, 2021
* Track all suspended work while it's still pending

This allows us to abort work and put everything into client rendered mode
if we don't want to wait for further I/O.

It also allows us to cancel fallbacks if we complete the main content
before the fallback.

* Expose abort API to the browser streams

Since this API already returns a value, we need to use destructuring to
expose more options.

* Add a test including the client actually client rendering it

* Use AbortSignal option for W3C streams instead of external control

* Clean up listener after it's used once
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants