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.

Sign up for GitHub

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

Jump to bottom

Replace global jest heuristic with IS_REACT_ACT_ENVIRONMENT #22562

Merged
merged 2 commits into from
Oct 18, 2021
Merged

Replace global jest heuristic with IS_REACT_ACT_ENVIRONMENT #22562

merged 2 commits into from
Oct 18, 2021

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Oct 14, 2021 •
edited
Loading

Refer to reactwg/react-18#102 for context on overall proposal

Based on #22561

In concurrent roots, instead of checking jest, we check the new IS_REACT_ACT_ENVIRONMENT global. The default behavior is false.

React's own internal test suite use a custom version of act that works by mocking the Scheduler — rather than the "real" act used publicly. So we don't enable the flag in our repo.

Also adds a warning if act is called but IS_REACT_ACT_ENVIRONMENT is not enabled. The goal is to prompt users to correctly configure their testing environment, so that if they forget to use act in a different test, we can detect and warn about.

It's expected that the environment flag will be configured by the testing framework. For example, a Jest plugin. We will link to the relevant documentation page, once it exists.

The new warning only fires in concurrent roots. Legacy roots will keep the existing behavior.

@facebook-github-bot facebook-github-bot added the React Core Team Opened by a member of the React Core Team label Oct 14, 2021
acdlite
acdlite commented Oct 14, 2021
View reviewed changes
packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js
@@ -134,9 +132,7 @@ describe('ReactDOMNativeEventHeuristic-test', () => {
// Dispatch a click event on the Disable-button.
const firstEvent = document.createEvent('Event');
firstEvent.initEvent('click', true, true);
expect(() => {
dispatchAndSetCurrentEvent(disableButton, firstEvent);
}).toErrorDev(['An update to Form inside a test was not wrapped in act']);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These warnings no longer fire because our tests don't enable the act environment flag — we mock the Scheduler package instead, or (as in this test) manually flush the microtask queue.

@sizebot
Copy link

sizebot commented Oct 14, 2021 •
edited
Loading

Comparing: 163e81c...80fb358

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.15 kB 130.15 kB = 41.41 kB 41.41 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 132.98 kB 132.98 kB = 42.39 kB 42.39 kB
facebook-www/ReactDOM-prod.classic.js = 414.32 kB 414.32 kB = 76.58 kB 76.58 kB
facebook-www/ReactDOM-prod.modern.js = 402.91 kB 402.91 kB = 74.85 kB 74.85 kB
facebook-www/ReactDOMForked-prod.classic.js = 414.32 kB 414.32 kB = 76.58 kB 76.58 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 80fb358

@acdlite acdlite force-pushed the replace-jest-heuristic branch from 4453969 to 04c4de3 Compare October 14, 2021 21:18
@acdlite acdlite marked this pull request as ready for review October 14, 2021 21:19
@acdlite acdlite requested a review from eps1lon October 14, 2021 21:19
eps1lon
eps1lon reviewed Oct 14, 2021
View reviewed changes
packages/react-reconciler/src/ReactFiberAct.new.js Outdated
// spurious warnings by setting IS_REACT_ACT_ENVIRONMENT explicitly
// to false.
//
// TODO: Only check `jest` in legacy mode. In concurrent mode, this heuristic
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this TODO supposed to be part of #22561 but not this PR?

This branch is only handled outside ConcurrentMode which implies to me "legacy mode". But maybe I'm off on the terminology?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I just neglected to delete the comment

@acdlite acdlite force-pushed the replace-jest-heuristic branch from 04c4de3 to 29a0fd3 Compare October 18, 2021 13:38
salazarm
salazarm approved these changes Oct 18, 2021
View reviewed changes
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

@acdlite
Remove jest global check in concurrent roots
5e74830 
In concurrent mode, instead of checking `jest`, we check the new
`IS_REACT_ACT_ENVIRONMENT` global. The default behavior is `false`.

Legacy mode behavior is unchanged.

React's own internal test suite use a custom version of `act` that works
by mocking the Scheduler — rather than the "real" act used publicly. So
we don't enable the flag in our repo.
@acdlite
Warn if act is called in wrong environment
80fb358 
Adds a warning if `act` is called but `IS_REACT_ACT_ENVIRONMENT` is not
enabled. The goal is to prompt users to correctly configure their
testing environment, so that if they forget to use `act` in a different
test, we can detect and warn about.

It's expected that the environment flag will be configured by the
testing framework. For example, a Jest plugin. We will link to the
relevant documentation page, once it exists.

The warning only fires in concurrent mode. Legacy roots will keep the
existing behavior.
@acdlite acdlite force-pushed the replace-jest-heuristic branch from 2f22a8b to 80fb358 Compare October 18, 2021 15:28
@acdlite acdlite merged commit 996da67 into facebook:main Oct 18, 2021
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Nov 2, 2021
@facebook-github-bot
React Native sync for revisions afcb9cd...3fcd81d
e161598 
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
@acdlite @KamranAsif
Replace global jest heuristic with IS_REACT_ACT_ENVIRONMENT (face…
9e456c6 
…book#22562)

* Remove `jest` global check in concurrent roots

In concurrent mode, instead of checking `jest`, we check the new
`IS_REACT_ACT_ENVIRONMENT` global. The default behavior is `false`.

Legacy mode behavior is unchanged.

React's own internal test suite use a custom version of `act` that works
by mocking the Scheduler — rather than the "real" act used publicly. So
we don't enable the flag in our repo.

* Warn if `act` is called in wrong environment

Adds a warning if `act` is called but `IS_REACT_ACT_ENVIRONMENT` is not
enabled. The goal is to prompt users to correctly configure their
testing environment, so that if they forget to use `act` in a different
test, we can detect and warn about.

It's expected that the environment flag will be configured by the
testing framework. For example, a Jest plugin. We will link to the
relevant documentation page, once it exists.

The warning only fires in concurrent mode. Legacy roots will keep the
existing behavior.
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Nov 15, 2021
@facebook-github-bot
React Native sync for revisions afcb9cd...c0c71a8
55663ec 
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 )**: [email protected] //<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 )**: [email protected] //<Dan Abramov>//
- **[827021c4e](facebook/react@827021c4e )**: Changelog for [email protected] //<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
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
@acdlite @zhengjitf
Replace global jest heuristic with IS_REACT_ACT_ENVIRONMENT (face…
ae50983 
…book#22562)

* Remove `jest` global check in concurrent roots

In concurrent mode, instead of checking `jest`, we check the new
`IS_REACT_ACT_ENVIRONMENT` global. The default behavior is `false`.

Legacy mode behavior is unchanged.

React's own internal test suite use a custom version of `act` that works
by mocking the Scheduler — rather than the "real" act used publicly. So
we don't enable the flag in our repo.

* Warn if `act` is called in wrong environment

Adds a warning if `act` is called but `IS_REACT_ACT_ENVIRONMENT` is not
enabled. The goal is to prompt users to correctly configure their
testing environment, so that if they forget to use `act` in a different
test, we can detect and warn about.

It's expected that the environment flag will be configured by the
testing framework. For example, a Jest plugin. We will link to the
relevant documentation page, once it exists.

The warning only fires in concurrent mode. Legacy roots will keep the
existing behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eps1lon eps1lon eps1lon left review comments

@crypt096 crypt096 crypt096 approved these changes

@salazarm salazarm salazarm approved these changes

Assignees
No one assigned
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@acdlite @sizebot @salazarm @eps1lon @crypt096 @facebook-github-bot