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

Expand act warning to cover all APIs that might schedule React work #22607

Merged
merged 4 commits into from
Oct 21, 2021

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Oct 20, 2021

In a test environment, React warns if an update isn't wrapped with act — but only if the update originates from a hook API, like useState.

We did it this way for backwards compatibility with tests that were written before the act API was introduced. Those tests didn't require act, anyway, because in a legacy root, all tasks are synchronous except for useEffect.

However, in a concurrent root, nearly every task is asynchronous. Even tasks that are synchronous may spawn additional asynchronous work. So all updates need to be wrapped with act, regardless of whether they originate from a hook, a class, a root, or any other type of component.

This PR expands the act warning to include any API that schedules React work.

Aside from updates, it also expands the warning to when a Suspense promise resolves. Both "pings" and "retries".

I also removed the check for whether an update is batched. It shouldn't matter, because even a batched update can spawn asynchronous work, which needs to be flushed by act.

Rollout plan

This change only affects concurrent roots. The behavior in legacy roots is the same.

The Relay test suite includes some concurrent tests. It's possible they may be affected. I'll look into that before merging.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Oct 20, 2021
@sizebot
Copy link

sizebot commented Oct 20, 2021

Comparing: fa9bea0...1961c99

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.19 kB 130.19 kB = 41.41 kB 41.42 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 135.16 kB 135.16 kB = 42.95 kB 42.95 kB
facebook-www/ReactDOM-prod.classic.js = 419.90 kB 419.90 kB = 77.33 kB 77.33 kB
facebook-www/ReactDOM-prod.modern.js = 408.48 kB 408.48 kB = 75.59 kB 75.59 kB
facebook-www/ReactDOMForked-prod.classic.js = 419.90 kB 419.90 kB = 77.34 kB 77.34 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
react-native/implementations/ReactFabric-dev.js +0.26% 728.73 kB 730.64 kB +0.28% 158.41 kB 158.85 kB
oss-stable-semver/react-art/cjs/react-art.development.js +0.26% 677.76 kB 679.51 kB +0.30% 147.02 kB 147.47 kB
oss-stable/react-art/cjs/react-art.development.js +0.26% 677.76 kB 679.51 kB +0.30% 147.02 kB 147.47 kB
facebook-www/ReactART-dev.modern.js +0.26% 744.63 kB 746.55 kB +0.28% 159.08 kB 159.52 kB
facebook-www/ReactART-dev.classic.js +0.25% 754.83 kB 756.74 kB +0.27% 161.17 kB 161.61 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-dev.js +0.25% 633.35 kB 634.95 kB +0.24% 137.38 kB 137.71 kB
react-native/implementations/ReactFabric-dev.fb.js +0.25% 762.05 kB 763.96 kB +0.27% 164.47 kB 164.91 kB
oss-experimental/react-art/cjs/react-art.development.js +0.25% 704.47 kB 706.22 kB +0.28% 152.77 kB 153.19 kB
facebook-www/ReactTestRenderer-dev.classic.js +0.24% 669.98 kB 671.59 kB +0.22% 144.24 kB 144.55 kB
facebook-www/ReactTestRenderer-dev.modern.js +0.24% 670.00 kB 671.60 kB +0.22% 144.25 kB 144.56 kB
oss-stable-semver/react-test-renderer/cjs/react-test-renderer.development.js +0.23% 620.49 kB 621.92 kB +0.24% 136.17 kB 136.49 kB
oss-stable/react-test-renderer/cjs/react-test-renderer.development.js +0.23% 620.49 kB 621.92 kB +0.24% 136.17 kB 136.49 kB
oss-stable-semver/react-art/umd/react-art.development.js +0.23% 780.76 kB 782.55 kB +0.26% 164.99 kB 165.43 kB
oss-stable/react-art/umd/react-art.development.js +0.23% 780.76 kB 782.55 kB +0.26% 164.99 kB 165.43 kB
oss-stable-semver/react-test-renderer/umd/react-test-renderer.development.js +0.23% 650.45 kB 651.92 kB +0.25% 137.68 kB 138.03 kB
oss-stable/react-test-renderer/umd/react-test-renderer.development.js +0.23% 650.45 kB 651.92 kB +0.25% 137.68 kB 138.03 kB
oss-experimental/react-art/umd/react-art.development.js +0.22% 808.77 kB 810.56 kB +0.24% 170.65 kB 171.06 kB
oss-experimental/react-test-renderer/cjs/react-test-renderer.development.js +0.22% 647.18 kB 648.61 kB +0.21% 141.88 kB 142.18 kB
oss-experimental/react-test-renderer/umd/react-test-renderer.development.js +0.22% 678.46 kB 679.93 kB +0.20% 143.47 kB 143.76 kB
react-native/implementations/ReactNativeRenderer-dev.js +0.22% 742.96 kB 744.56 kB +0.23% 161.75 kB 162.12 kB
react-native/implementations/ReactNativeRenderer-dev.fb.js +0.21% 774.31 kB 775.91 kB +0.20% 167.49 kB 167.83 kB

Generated by 🚫 dangerJS against 1961c99

Copy link
Contributor

@salazarm salazarm left a comment

Choose a reason for hiding this comment

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

nice

I'm about to fork the behavior in legacy roots versus concurrent roots
even further, so I'm lifting this up so I only have to fork once.
Similar to previous commit. I only want to check this once. Not for
performance reasons, but so the logic is easier to follow.
In a test environment, React warns if an update isn't wrapped with act
— but only if the update originates from a hook API, like useState.

We did it this way for backwards compatibility with tests that were
written before the act API was introduced. Those tests didn't require
act, anyway, because in a legacy root, all tasks are synchronous except
for `useEffect`.

However, in a concurrent root, nearly every task is asynchronous. Even
tasks that are synchronous may spawn additional asynchronous work. So
all updates need to be wrapped with act, regardless of whether they
originate from a hook, a class, a root, or any other type of component.

This commit expands the act warning to include any API that triggers
an update.

It does not currently account for renders that are caused by a Suspense
promise resolving; those are modelled slightly differently from updates.
I'll fix that in the next step.

I also removed the check for whether an update is batched. It shouldn't
matter, because even a batched update can spawn asynchronous work, which
needs to be flushed by act.

This change only affects concurrent roots. The behavior in legacy roots
is the same.
For the same reason we warn when an update is not wrapped with act,
we should warn if a Suspense promise resolution is not wrapped with act.
Both "pings" and "retries".

Legacy root behavior is unchanged.
@acdlite acdlite force-pushed the expand-act-warning branch from 21cc354 to 1961c99 Compare October 21, 2021 22:28
@acdlite acdlite merged commit d5b6b4b into facebook:main Oct 21, 2021
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Nov 2, 2021
Summary:
This sync includes the following changes:
- **[5cccacd13](facebook/react@5cccacd13 )**: Upgrade useId to alpha channel ([#22674](facebook/react#22674)) //<Andrew Clark>//
- **[75f3ddebf](facebook/react@75f3ddebf )**: Remove experimental useOpaqueIdentifier API ([#22672](facebook/react#22672)) //<Andrew Clark>//
- **[8c4a05b8f](facebook/react@8c4a05b8f )**: Remove flow pragma comment from module registration start/stop templates ([#22670](facebook/react#22670)) //<Brian Vaughn>//
- **[ebf9ae857](facebook/react@ebf9ae857 )**: useId ([#22644](facebook/react#22644)) //<Andrew Clark>//
- **[a0d991fe6](facebook/react@a0d991fe6 )**: Re-land #22292 (remove uMS from open source build) ([#22664](facebook/react#22664)) //<Andrew Clark>//
- **[6bce0355c](facebook/react@6bce0355c )**: Upgrade useSyncExternalStore to alpha channel ([#22662](facebook/react#22662)) //<Andrew Clark>//
- **[7034408ff](facebook/react@7034408ff )**: Follow-up improvements to error code extraction infra ([#22516](facebook/react#22516)) //<Andrew Clark>//
- **[90e5d3638](facebook/react@90e5d3638 )**: chore: fix comment typo ([#22615](facebook/react#22615)) //<btea>//
- **[3c4c1c470](facebook/react@3c4c1c470 )**: Remove warning for dangling passive effects ([#22609](facebook/react#22609)) //<Andrew Clark>//
- **[d5b6b4b86](facebook/react@d5b6b4b86 )**: Expand act warning to cover all APIs that might schedule React work ([#22607](facebook/react#22607)) //<Andrew Clark>//
- **[fa9bea0c4](facebook/react@fa9bea0c4 )**: Initial implementation of cache cleanup ([#22510](facebook/react#22510)) //<Joseph Savona>//
- **[0e8a5aff3](facebook/react@0e8a5aff3 )**: Scheduling Profiler: Add marks for component effects (mount and unmount) ([#22578](facebook/react#22578)) //<Brian Vaughn>//
- **[4ba20579d](facebook/react@4ba20579d )**: Scheduling Profiler: De-emphasize React internal frames ([#22588](facebook/react#22588)) //<Brian Vaughn>//
- **[cdb8a1d19](facebook/react@cdb8a1d19 )**: [Fizz] Add option to inject bootstrapping script tags after the shell is injected ([#22594](facebook/react#22594)) //<Sebastian Markbåge>//
- **[34e4c9756](facebook/react@34e4c9756 )**: Clear extra nodes if there's a hydration mismatch within a suspense boundary  ([#22592](facebook/react#22592)) //<Sebastian Markbåge>//
- **[02f411578](facebook/react@02f411578 )**: Upgrade useInsertionEffect to stable ([#22589](facebook/react#22589)) //<Andrew Clark>//
- **[2af4a7933](facebook/react@2af4a7933 )**: Hydrate using SuspenseComponent as the parent ([#22582](facebook/react#22582)) //<Sebastian Markbåge>//
- **[b1acff0cc](facebook/react@b1acff0cc )**: Enable cache in test renderer ([#22580](facebook/react#22580)) //<Joseph Savona>//
- **[996da67b2](facebook/react@996da67b2 )**: Replace global `jest` heuristic with `IS_REACT_ACT_ENVIRONMENT` ([#22562](facebook/react#22562)) //<Andrew Clark>//
- **[163e81c1f](facebook/react@163e81c1f )**: Support disabling spurious act warnings with a global environment flag ([#22561](facebook/react#22561)) //<Andrew Clark>//
- **[23b7dfeff](facebook/react@23b7dfeff )**: Enable scheduling profiler for RN FB profiling builds ([#22566](facebook/react#22566)) //<Brian Vaughn>//
- **[61455a25b](facebook/react@61455a25b )**: Enable experimental Cache API in www TestRenderer ([#22554](facebook/react#22554)) //<Joseph Savona>//
- **[7142d110b](facebook/react@7142d110b )**: Bugfix: Nested useOpaqueIdentifier references ([#22553](facebook/react#22553)) //<Andrew Clark>//
- **[1e247ff89](facebook/react@1e247ff89 )**: Enabled scheduling profiler marks for React Native FB target ([#22544](facebook/react#22544)) //<Brian Vaughn>//
- **[c16b005f2](facebook/react@c16b005f2 )**: Update test and stack frame code to support newer V8 stack formats ([#22477](facebook/react#22477)) //<Brian Vaughn>//
- **[55d75005b](facebook/react@55d75005b )**: duplicate value in variable ([#22390](facebook/react#22390)) //<BIKI DAS>//

Changelog:
[General][Changed] - React Native sync for revisions afcb9cd...3fcd81d

jest_e2e[run_all_tests]

Reviewed By: yungsters

Differential Revision: D32065987

fbshipit-source-id: ef2d402835c981aab68ca40a894c66c1630864e9
KamranAsif pushed a commit to KamranAsif/react that referenced this pull request Nov 4, 2021
…acebook#22607)

* Move isActEnvironment check to function that warns

I'm about to fork the behavior in legacy roots versus concurrent roots
even further, so I'm lifting this up so I only have to fork once.

* Lift `mode` check, too

Similar to previous commit. I only want to check this once. Not for
performance reasons, but so the logic is easier to follow.

* Expand act warning to include non-hook APIs

In a test environment, React warns if an update isn't wrapped with act
— but only if the update originates from a hook API, like useState.

We did it this way for backwards compatibility with tests that were
written before the act API was introduced. Those tests didn't require
act, anyway, because in a legacy root, all tasks are synchronous except
for `useEffect`.

However, in a concurrent root, nearly every task is asynchronous. Even
tasks that are synchronous may spawn additional asynchronous work. So
all updates need to be wrapped with act, regardless of whether they
originate from a hook, a class, a root, or any other type of component.

This commit expands the act warning to include any API that triggers
an update.

It does not currently account for renders that are caused by a Suspense
promise resolving; those are modelled slightly differently from updates.
I'll fix that in the next step.

I also removed the check for whether an update is batched. It shouldn't
matter, because even a batched update can spawn asynchronous work, which
needs to be flushed by act.

This change only affects concurrent roots. The behavior in legacy roots
is the same.

* Expand act warning to include Suspense resolutions

For the same reason we warn when an update is not wrapped with act,
we should warn if a Suspense promise resolution is not wrapped with act.
Both "pings" and "retries".

Legacy root behavior is unchanged.
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Nov 15, 2021
Summary:
This sync includes the following changes:
- **[c0c71a868](facebook/react@c0c71a868 )**: Re-enable useMutableSource in internal RN ([#22750](facebook/react#22750)) //<Ricky>//
- **[cb11155c8](facebook/react@cb11155c8 )**: Add runtime type checks around module boundary code ([#22748](facebook/react#22748)) //<Brian Vaughn>//
- **[a04f13d29](facebook/react@a04f13d29 )**: react-refresh@0.11.0 //<Dan Abramov>//
- **[ff9897d23](facebook/react@ff9897d23 )**: [React Refresh] support typescript namespace syntax ([#22621](facebook/react#22621)) //<irinakk>//
- **[0ddd69d12](facebook/react@0ddd69d12 )**: Throw on hydration mismatch and force client rendering if boundary hasn't suspended within concurrent root ([#22629](facebook/react#22629)) //<salazarm>//
- **[c3f34e4be](facebook/react@c3f34e4be )**: eslint-plugin-react-hooks@4.3.0 //<Dan Abramov>//
- **[827021c4e](facebook/react@827021c4e )**: Changelog for eslint-plugin-react-hooks@4.3.0 //<Dan Abramov>//
- **[8ca3f567b](facebook/react@8ca3f567b )**: Fix module-boundary wrappers ([#22688](facebook/react#22688)) //<Brian Vaughn>//
- **[1bf6deb86](facebook/react@1bf6deb86 )**: Renamed packages/react-devtools-scheduling-profiler to packages/react-devtools-timeline ([#22691](facebook/react#22691)) //<Brian Vaughn>//
- **[51c558aeb](facebook/react@51c558aeb )**: Rename (some) "scheduling profiler" references to "timeline" ([#22690](facebook/react#22690)) //<Brian Vaughn>//
- **[00ced1e2b](facebook/react@00ced1e2b )**: Fix useId in strict mode ([#22681](facebook/react#22681)) //<Andrew Clark>//
- **[5cccacd13](facebook/react@5cccacd13 )**: Upgrade useId to alpha channel ([#22674](facebook/react#22674)) //<Andrew Clark>//
- **[75f3ddebf](facebook/react@75f3ddebf )**: Remove experimental useOpaqueIdentifier API ([#22672](facebook/react#22672)) //<Andrew Clark>//
- **[8c4a05b8f](facebook/react@8c4a05b8f )**: Remove flow pragma comment from module registration start/stop templates ([#22670](facebook/react#22670)) //<Brian Vaughn>//
- **[ebf9ae857](facebook/react@ebf9ae857 )**: useId ([#22644](facebook/react#22644)) //<Andrew Clark>//
- **[a0d991fe6](facebook/react@a0d991fe6 )**: Re-land #22292 (remove uMS from open source build) ([#22664](facebook/react#22664)) //<Andrew Clark>//
- **[6bce0355c](facebook/react@6bce0355c )**: Upgrade useSyncExternalStore to alpha channel ([#22662](facebook/react#22662)) //<Andrew Clark>//
- **[7034408ff](facebook/react@7034408ff )**: Follow-up improvements to error code extraction infra ([#22516](facebook/react#22516)) //<Andrew Clark>//
- **[90e5d3638](facebook/react@90e5d3638 )**: chore: fix comment typo ([#22615](facebook/react#22615)) //<btea>//
- **[3c4c1c470](facebook/react@3c4c1c470 )**: Remove warning for dangling passive effects ([#22609](facebook/react#22609)) //<Andrew Clark>//
- **[d5b6b4b86](facebook/react@d5b6b4b86 )**: Expand act warning to cover all APIs that might schedule React work ([#22607](facebook/react#22607)) //<Andrew Clark>//
- **[fa9bea0c4](facebook/react@fa9bea0c4 )**: Initial implementation of cache cleanup ([#22510](facebook/react#22510)) //<Joseph Savona>//
- **[0e8a5aff3](facebook/react@0e8a5aff3 )**: Scheduling Profiler: Add marks for component effects (mount and unmount) ([#22578](facebook/react#22578)) //<Brian Vaughn>//
- **[4ba20579d](facebook/react@4ba20579d )**: Scheduling Profiler: De-emphasize React internal frames ([#22588](facebook/react#22588)) //<Brian Vaughn>//
- **[cdb8a1d19](facebook/react@cdb8a1d19 )**: [Fizz] Add option to inject bootstrapping script tags after the shell is injected ([#22594](facebook/react#22594)) //<Sebastian Markbåge>//
- **[34e4c9756](facebook/react@34e4c9756 )**: Clear extra nodes if there's a hydration mismatch within a suspense boundary  ([#22592](facebook/react#22592)) //<Sebastian Markbåge>//
- **[02f411578](facebook/react@02f411578 )**: Upgrade useInsertionEffect to stable ([#22589](facebook/react#22589)) //<Andrew Clark>//
- **[2af4a7933](facebook/react@2af4a7933 )**: Hydrate using SuspenseComponent as the parent ([#22582](facebook/react#22582)) //<Sebastian Markbåge>//
- **[b1acff0cc](facebook/react@b1acff0cc )**: Enable cache in test renderer ([#22580](facebook/react#22580)) //<Joseph Savona>//
- **[996da67b2](facebook/react@996da67b2 )**: Replace global `jest` heuristic with `IS_REACT_ACT_ENVIRONMENT` ([#22562](facebook/react#22562)) //<Andrew Clark>//
- **[163e81c1f](facebook/react@163e81c1f )**: Support disabling spurious act warnings with a global environment flag ([#22561](facebook/react#22561)) //<Andrew Clark>//
- **[23b7dfeff](facebook/react@23b7dfeff )**: Enable scheduling profiler for RN FB profiling builds ([#22566](facebook/react#22566)) //<Brian Vaughn>//
- **[61455a25b](facebook/react@61455a25b )**: Enable experimental Cache API in www TestRenderer ([#22554](facebook/react#22554)) //<Joseph Savona>//
- **[7142d110b](facebook/react@7142d110b )**: Bugfix: Nested useOpaqueIdentifier references ([#22553](facebook/react#22553)) //<Andrew Clark>//
- **[1e247ff89](facebook/react@1e247ff89 )**: Enabled scheduling profiler marks for React Native FB target ([#22544](facebook/react#22544)) //<Brian Vaughn>//
- **[c16b005f2](facebook/react@c16b005f2 )**: Update test and stack frame code to support newer V8 stack formats ([#22477](facebook/react#22477)) //<Brian Vaughn>//
- **[55d75005b](facebook/react@55d75005b )**: duplicate value in variable ([#22390](facebook/react#22390)) //<BIKI DAS>//

Changelog:
[General][Changed] - React Native sync for revisions afcb9cd...c0c71a8

jest_e2e[run_all_tests]

Reviewed By: yungsters

Differential Revision: D32395873

fbshipit-source-id: 3afd158f167b1eedcc244e29aba1a2c502d3c9d9
@gaearon gaearon mentioned this pull request Mar 29, 2022
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
…acebook#22607)

* Move isActEnvironment check to function that warns

I'm about to fork the behavior in legacy roots versus concurrent roots
even further, so I'm lifting this up so I only have to fork once.

* Lift `mode` check, too

Similar to previous commit. I only want to check this once. Not for
performance reasons, but so the logic is easier to follow.

* Expand act warning to include non-hook APIs

In a test environment, React warns if an update isn't wrapped with act
— but only if the update originates from a hook API, like useState.

We did it this way for backwards compatibility with tests that were
written before the act API was introduced. Those tests didn't require
act, anyway, because in a legacy root, all tasks are synchronous except
for `useEffect`.

However, in a concurrent root, nearly every task is asynchronous. Even
tasks that are synchronous may spawn additional asynchronous work. So
all updates need to be wrapped with act, regardless of whether they
originate from a hook, a class, a root, or any other type of component.

This commit expands the act warning to include any API that triggers
an update.

It does not currently account for renders that are caused by a Suspense
promise resolving; those are modelled slightly differently from updates.
I'll fix that in the next step.

I also removed the check for whether an update is batched. It shouldn't
matter, because even a batched update can spawn asynchronous work, which
needs to be flushed by act.

This change only affects concurrent roots. The behavior in legacy roots
is the same.

* Expand act warning to include Suspense resolutions

For the same reason we warn when an update is not wrapped with act,
we should warn if a Suspense promise resolution is not wrapped with act.
Both "pings" and "retries".

Legacy root behavior is unchanged.
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