Add fallback shim for AbortController #24285

rickhanlonii · 2022-04-06T20:41:28Z

Adds a lightweight fallback shim for environments that don't have an AbortController (e.g. tests running in Node < 15).

gaearon

Even if it's only on for enableCache builds, I think we shouldn't ship this in any browser bundles.

packages/react-reconciler/src/ReactAbortController.js

josephsavona · 2022-04-07T17:08:35Z

Even if it's only on for enableCache builds, I think we shouldn't ship this in any browser bundles.

This seems reasonable to me — is this concern addressed? My naive understanding of packaging suggests the code would still be included but maybe i'm overlooking something.

acdlite

I wouldn't think of this as implementing AbortController. It's more about we need fallback "signal" implementation that we can return if the native AbortController isn't available.

The signal object should have:

addEventListener that only supports a single event type (abort)
a boolean aborted field

and that's it. We don't need all the indirection of a controller object because the controller is only an implementation detail.

The thing to optimize for here is code size, not performance. What's the smallest possible implementation you can write? I probably wouldn't even put in a separate file, just inline it into the cache signal implementation.

packages/react-reconciler/src/ReactAbortController.js

rickhanlonii · 2022-04-07T17:30:57Z

@acdlite we also need dispatchEvent and to call the registered listeners, correct?

acdlite · 2022-04-07T17:32:20Z

If in the end the code size is too large, we can have it fall back to returning null or something instead, then come back to this if/when the feature is closer to shipping.

The first order purpose of this task is for the experimental React build to not break if you run it in a node/test environment. So if getCacheSignal returns null that's fine, since nobody is using it yet.

acdlite · 2022-04-07T17:32:47Z

@acdlite we also need dispatchEvent and to call the registered listeners, correct?

React can call the listeners directly without going through a dispatchEvent indirection

packages/react-reconciler/src/ReactFiberCacheComponent.new.js

sizebot · 2022-04-07T18:05:08Z

Comparing: ebd7ff6...afe0c82

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	=	131.30 kB	131.30 kB	=	41.97 kB	41.97 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	+0.15%	136.36 kB	136.57 kB	+0.19%	43.42 kB	43.50 kB
facebook-www/ReactDOM-prod.classic.js	+0.13%	432.75 kB	433.31 kB	+0.18%	79.58 kB	79.72 kB
facebook-www/ReactDOM-prod.modern.js	+0.13%	417.75 kB	418.30 kB	+0.18%	77.21 kB	77.35 kB
facebook-www/ReactDOMForked-prod.classic.js	+0.13%	432.75 kB	433.31 kB	+0.18%	79.58 kB	79.72 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-art/cjs/react-art.production.min.js	+0.24%	86.49 kB	86.69 kB	+0.31%	26.69 kB	26.78 kB
oss-experimental/react-test-renderer/cjs/react-test-renderer.production.min.js	+0.23%	88.25 kB	88.46 kB	+0.35%	27.13 kB	27.22 kB
oss-experimental/react-test-renderer/umd/react-test-renderer.production.min.js	+0.23%	88.45 kB	88.65 kB	+0.31%	27.37 kB	27.45 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-prod.js	+0.22%	247.59 kB	248.14 kB	+0.32%	45.26 kB	45.40 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-profiling.js	+0.21%	262.37 kB	262.93 kB	+0.30%	47.60 kB	47.74 kB
oss-experimental/react-reconciler/cjs/react-reconciler.production.min.js	+0.21%	97.34 kB	97.54 kB	+0.30%	29.68 kB	29.77 kB
facebook-www/ReactART-prod.modern.js	+0.21%	267.20 kB	267.75 kB	+0.27%	47.60 kB	47.73 kB

Generated by 🚫 dangerJS against afe0c82

rickhanlonii · 2022-04-07T19:16:05Z

Updated based on feedback, thanks all!

@acdlite is that more in line with what you were thinking?

@josephsavona @gaearon is this small enough to ease your concerns?

gaearon

Seems ok but we should add to the umbrella to drop this in 19 together with support for running tests in old Node. 16 is already LTS.

josephsavona · 2022-04-07T20:49:51Z

shipit

acdlite

I left a nit here but other than that looks good to me! https://github.com/facebook/react/pull/24285/files#r845539601

gaearon · 2022-04-08T16:08:58Z

Hmm by the history of sizebot edits the last version is a tiny bit larger.

acdlite · 2022-04-08T17:32:49Z

Probably because of the createController indirection, which really only exists because you have to new AbortController

acdlite · 2022-04-08T17:48:09Z

One nice thing about the fake class implementation, despite the non-mangled names, is that the normal path (no fallback) doesn't have any additional runtime overhead (the createController indirection in this case, which is annoying because it only exists because of the new). Just the one typecheck during module initialization. You can manually mangle the names like @gaearon suggested to code golf it some more. At this point though we're talking about 1 or 2 bytes difference so whichever you prefer.

rickhanlonii · 2022-04-08T17:51:48Z

Yeah @acdlite good point.

I reverted back to the object constructor and squeezed out a little more by dropping this.listeners. I think that's all we can change since names like abort, aborted and addEventListener are required.

* Add fallback shim for AbortController * Replace shim with a minimal stub * replace-fork * Better minification * Fix flow * Even smaller * replace-fork * Revert back to object constructor * replace-fork

Summary: This sync includes the following changes: - **[8dcedba15](facebook/react@8dcedba15 )**: Add fallback shim for AbortController ([#24285](facebook/react#24285)) //<Ricky>// - **[b86baa1cb](facebook/react@b86baa1cb )**: Add back lost cache test ([#24317](facebook/react#24317)) //<Ricky>// - **[bafe912a5](facebook/react@bafe912a5 )**: update types for InputContinuousLane and DefaultLane ([#24316](facebook/react#24316)) //<Leo>// - **[4ebaeae40](facebook/react@4ebaeae40 )**: moved mutation code to passive ([#24251](facebook/react#24251)) //<Luna Ruan>// - **[caa60e8fc](facebook/react@caa60e8fc )**: update types for NonIdleLanes and IdleLane ([#24313](facebook/react#24313)) //<Leo>// - **[1f7a901d7](facebook/react@1f7a901d7 )**: Fix false positive lint error with large number of branches ([#24287](facebook/react#24287)) //<Stephen Cyron>// - **[f56dfe950](facebook/react@f56dfe950 )**: Warn on setState() in useInsertionEffect() ([#24298](facebook/react#24298)) //<dan>// - **[d68b09def](facebook/react@d68b09def )**: Fix warning about setState in useEffect ([#24295](facebook/react#24295)) //<dan>// - **[057915477](facebook/react@057915477 )**: Update create-subscription README ([#24294](facebook/react#24294)) //<dan>// Changelog: [General][Changed] - React Native sync for revisions e8f4a66...8dcedba jest_e2e[run_all_tests] Reviewed By: kacieb Differential Revision: D35581147 fbshipit-source-id: 33661d77eb000fdedab7e506a458fc739eab0056

* Add fallback shim for AbortController * Replace shim with a minimal stub * replace-fork * Better minification * Fix flow * Even smaller * replace-fork * Revert back to object constructor * replace-fork

Summary: This sync includes the following changes: - **[8dcedba15](facebook/react@8dcedba15 )**: Add fallback shim for AbortController ([facebook#24285](facebook/react#24285)) //<Ricky>// - **[b86baa1cb](facebook/react@b86baa1cb )**: Add back lost cache test ([facebook#24317](facebook/react#24317)) //<Ricky>// - **[bafe912a5](facebook/react@bafe912a5 )**: update types for InputContinuousLane and DefaultLane ([facebook#24316](facebook/react#24316)) //<Leo>// - **[4ebaeae40](facebook/react@4ebaeae40 )**: moved mutation code to passive ([facebook#24251](facebook/react#24251)) //<Luna Ruan>// - **[caa60e8fc](facebook/react@caa60e8fc )**: update types for NonIdleLanes and IdleLane ([facebook#24313](facebook/react#24313)) //<Leo>// - **[1f7a901d7](facebook/react@1f7a901d7 )**: Fix false positive lint error with large number of branches ([facebook#24287](facebook/react#24287)) //<Stephen Cyron>// - **[f56dfe950](facebook/react@f56dfe950 )**: Warn on setState() in useInsertionEffect() ([facebook#24298](facebook/react#24298)) //<dan>// - **[d68b09def](facebook/react@d68b09def )**: Fix warning about setState in useEffect ([facebook#24295](facebook/react#24295)) //<dan>// - **[057915477](facebook/react@057915477 )**: Update create-subscription README ([facebook#24294](facebook/react#24294)) //<dan>// Changelog: [General][Changed] - React Native sync for revisions e8f4a66...8dcedba jest_e2e[run_all_tests] Reviewed By: kacieb Differential Revision: D35581147 fbshipit-source-id: 33661d77eb000fdedab7e506a458fc739eab0056

rickhanlonii requested review from acdlite and josephsavona April 6, 2022 20:41

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 6, 2022

gaearon requested changes Apr 6, 2022

View reviewed changes

rickhanlonii force-pushed the rh-abort-controller branch from d89603a to 646ebad Compare April 6, 2022 21:04

rickhanlonii commented Apr 6, 2022

View reviewed changes

packages/react-reconciler/src/ReactAbortController.js Outdated Show resolved Hide resolved

Add fallback shim for AbortController

6745d48

rickhanlonii force-pushed the rh-abort-controller branch from 646ebad to 6745d48 Compare April 6, 2022 21:09

acdlite requested changes Apr 7, 2022

View reviewed changes

packages/react-reconciler/src/ReactAbortController.js Outdated Show resolved Hide resolved

rickhanlonii added 2 commits April 7, 2022 13:50

Replace shim with a minimal stub

fc32cff

replace-fork

514ab3e

gaearon reviewed Apr 7, 2022

View reviewed changes

packages/react-reconciler/src/ReactFiberCacheComponent.new.js Show resolved Hide resolved

rickhanlonii added 2 commits April 7, 2022 14:53

Better minification

6cb52c8

Fix flow

8d50df6

gaearon approved these changes Apr 7, 2022

View reviewed changes

acdlite approved these changes Apr 7, 2022

View reviewed changes

rickhanlonii added 2 commits April 8, 2022 11:15

Even smaller

5b6f406

replace-fork

d038e29

Revert back to object constructor

492474b

replace-fork

afe0c82

rickhanlonii merged commit 8dcedba into facebook:main Apr 8, 2022

rickhanlonii deleted the rh-abort-controller branch April 8, 2022 19:53

ymqy mentioned this pull request Jan 31, 2023

Remove unused dependency 'abort-controller' #26074

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fallback shim for AbortController #24285

Add fallback shim for AbortController #24285

rickhanlonii commented Apr 6, 2022

gaearon left a comment

josephsavona commented Apr 7, 2022

acdlite left a comment

rickhanlonii commented Apr 7, 2022

acdlite commented Apr 7, 2022 •

edited

acdlite commented Apr 7, 2022

sizebot commented Apr 7, 2022 •

edited

rickhanlonii commented Apr 7, 2022 •

edited

gaearon left a comment

josephsavona commented Apr 7, 2022

acdlite left a comment

gaearon commented Apr 8, 2022

acdlite commented Apr 8, 2022

acdlite commented Apr 8, 2022 •

edited

rickhanlonii commented Apr 8, 2022

Add fallback shim for AbortController #24285

Add fallback shim for AbortController #24285

Conversation

rickhanlonii commented Apr 6, 2022

gaearon left a comment

Choose a reason for hiding this comment

josephsavona commented Apr 7, 2022

acdlite left a comment

Choose a reason for hiding this comment

rickhanlonii commented Apr 7, 2022

acdlite commented Apr 7, 2022 • edited

acdlite commented Apr 7, 2022

sizebot commented Apr 7, 2022 • edited

Critical size changes

Significant size changes

rickhanlonii commented Apr 7, 2022 • edited

gaearon left a comment

Choose a reason for hiding this comment

josephsavona commented Apr 7, 2022

acdlite left a comment

Choose a reason for hiding this comment

gaearon commented Apr 8, 2022

acdlite commented Apr 8, 2022

acdlite commented Apr 8, 2022 • edited

rickhanlonii commented Apr 8, 2022

acdlite commented Apr 7, 2022 •

edited

sizebot commented Apr 7, 2022 •

edited

rickhanlonii commented Apr 7, 2022 •

edited

acdlite commented Apr 8, 2022 •

edited