warn if passive effects get queued outside of an act() call. #15763
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
threepointone
commented
May 29, 2019
threepointone
commented
May 29, 2019
packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js
Outdated
Show resolved
Hide resolved
Details of bundled changes.Comparing: 34ce57a...8bcfcc3 react-dom
react-art
react-test-renderer
react-native-renderer
react-reconciler
Generated by 🚫 dangerJS |
threepointone
commented
May 30, 2019
packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
Outdated
Show resolved
Hide resolved
acdlite
approved these changes
Jun 11, 2019
packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
Outdated
Show resolved
Hide resolved
|
working on this, changes planned. |
threepointone
force-pushed
the
unacted-effects-warn
branch
from
1757db6
to
afdad24
Compare
While the code itself isn't much (it adds the warning to mountEffect() and updateEffect() in ReactFiberHooks), it does change a lot of our tests. We follow a bad-ish pattern here, which is doing asserts inside act() scopes, but it makes sense for *us* because we're testing intermediate states, and we're manually flush/yield what we need in these tests. This commit has one last failing test. Working on it.
- a test was failing in ReactDOMServerIntegrationHooks while testing an effect; the behaviour of yields was different from browser and server when wrapped with act(). further, because of how we initialized modules, act() around renders wasn't working corrrectly. solved by passing in ReactTestUtils in initModules, and checking on the finally yielded values in the specific test. - in ReactUpdates, while testing an infinite recursion detection, the test needed to be wrapped in an act(), which would have caused the recusrsion error to throw. solived by rethrowing the error from inside the act().
threepointone
force-pushed
the
unacted-effects-warn
branch
from
afdad24
to
46c0741
Compare
|
fixed the failing test, and amended the other one; see this commit for details 46c0741 I'll work on a better warning message and land this soon. |
threepointone
commented
Jun 23, 2019
threepointone
force-pushed
the
unacted-effects-warn
branch
from
5b07b1a
to
b8202ba
Compare
threepointone
force-pushed
the
unacted-effects-warn
branch
from
b8202ba
to
8bcfcc3
Compare
threepointone
added a commit
to threepointone/react
that referenced
this pull request
Jun 24, 2019
It was me. I broke the build.
threepointone
added a commit
that referenced
this pull request
Jun 24, 2019
* followup to #15763, failing tests in ReactDOMTracing-test It was me. I broke the build. * [ignore] add a newline to trigger a build
rickhanlonii
pushed a commit
to rickhanlonii/react
that referenced
this pull request
Jun 25, 2019
…k#15763) * warn if passive effects get queued outside of an act() call While the code itself isn't much (it adds the warning to mountEffect() and updateEffect() in ReactFiberHooks), it does change a lot of our tests. We follow a bad-ish pattern here, which is doing asserts inside act() scopes, but it makes sense for *us* because we're testing intermediate states, and we're manually flush/yield what we need in these tests. This commit has one last failing test. Working on it. * pass lint * pass failing test, fixes another - a test was failing in ReactDOMServerIntegrationHooks while testing an effect; the behaviour of yields was different from browser and server when wrapped with act(). further, because of how we initialized modules, act() around renders wasn't working corrrectly. solved by passing in ReactTestUtils in initModules, and checking on the finally yielded values in the specific test. - in ReactUpdates, while testing an infinite recursion detection, the test needed to be wrapped in an act(), which would have caused the recusrsion error to throw. solived by rethrowing the error from inside the act(). * pass ReactDOMServerSuspense * stray todo * a better message, consistent with the state update one.
rickhanlonii
pushed a commit
to rickhanlonii/react
that referenced
this pull request
Jun 25, 2019
…acebook#15972) * followup to facebook#15763, failing tests in ReactDOMTracing-test It was me. I broke the build. * [ignore] add a newline to trigger a build
Merged
This was referenced Mar 10, 2020
This was referenced Mar 18, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds a warning when effects are queued outside of an act() scope. Please see this diff with whitespace off (add
?w=1to your url)While the code itself isn't much (it adds the warning to
mountEffect()andupdateEffect()inReactFiberHooks), it does change a lot of our tests. We follow a bad-ish pattern here, which is doing asserts insideact()scopes, but it makes sense for us because we're testing intermediate states, and we're manually flush/yield what we need in these tests.This commit has one last failing test. Working on it, might reach out to one of you for help.
Please please look closely at the tests I've changed; I went about it with a rough-ish hand, wrapping whatever started warning with
act()calls. For bits that seemed a bit hard to wrap withact()without drastically changing the test, I explicitly caught the missingact()warnings withexpect(...).toWarnDev([...])statements.An important question - Is this worth it? Consider an app built with both classical, and functional components. Assume that the class ones are parents to functional children. As we add hooks and effects to children, tests that used to pass for class components will start failing, meaning people will have to update tests for older components they might not be interested in touching.