warn if passive effects get queued outside of an act() call. #15631
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
added 11 commits
May 8, 2019 18:05
a first crack at flushing the scheduler manually from inside act(). uses unstable_flushWithoutYielding(). The tests that changes, mostly replaced toFlushAndYield(...) with toHaveYielded(). For some tests that tested the state of the tree before flushing effects (but still after updates), I replaced act() with bacthedUpdates().
of note, unstable_flushWithoutYielding now returns a boolean much like flushPassiveEffects
…shWithoutYielding
I also tried writing a test for it, but couldn't get the scheduler to unmock. included the failing test.
based on facebook#15591. of note, we don't modify our own tests to satisfy this warning, instead using a feature flag to disable the warning for some tests. this is because we're testing the actual sequencing of work in these tests, and don't want to flush everything with act().
|
React: size: 0.0%, gzip: -0.0% Details of bundled changes.Comparing: c7398f3...9f7a346 react
react-dom
react-art
react-native-renderer
react-test-renderer
react-noop-renderer
react-reconciler
scheduler
Generated by 🚫 dangerJS |
acdlite
reviewed
May 15, 2019
| if (ReactShouldWarnActingUpdates.current === false) { | ||
| warningWithoutStack( | ||
| false, | ||
| 'An effect from %s inside a test was not wrapped in act(...).\n\n' + |
There was a problem hiding this comment.
Nit: This makes it sound like useEffect is supposed to be wrapped in act.
acdlite
approved these changes
May 15, 2019
|
^ approved once base PR is merged |
4 tasks
|
I'm rewriting this to (hopefully) not need the feature flag. it'll mean rewriting a lot of the tests. |
|
@threepointone Let's land the change before updating a bunch of tests? I would prefer not to couple those. |
|
made an attempt at doing this without the feature flag here - #15763 |
|
abandoning this for #15763 |
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 version uses a feature flag to disable the warning in tests. It isn't a viable options since we'd have to disable a whole lot of tests that run in yarn test-build See #15763 instead
based on #15591. this might be a bit controversial, but I agree with the reasoning. of note, we don't modify our own tests to satisfy this warning, instead using a feature flag to disable the warning for some tests. this is because we're testing the actual sequencing of work in these tests, and don't want to flush everything with act().
you probably want to see just this commit - 9f7a346