Delete flushSuspenseFallbacksInTests flag #18596
Merged
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
This was meant to be a temporary hack to unblock the `act` work, but it quickly spread throughout our tests. What it's meant to do is force fallbacks to flush inside `act` even in Concurrent Mode. It does this by wrapping the `setTimeout` call in a check to see if it's in an `act` context. If so, it skips the delay and immediately commits the fallback. Really this is only meant for our internal React tests that need to incrementally render. Nobody outside our team (and Relay) needs to do that, yet. Even if/when we do support that, it may or may not be with the same `flushAndYield` pattern we use internally. However, even for our internal purposes, the behavior isn't right because a really common reason we flush work incrementally is to make assertions on the "suspended" state, before the fallback has committed. There's no way to do that from inside `act` with the behavior of this flag, because it causes the fallback to immediately commit. This has led us to *not* use `act` in a lot of our tests, or to write code that doesn't match what would actually happen in a real environment. What we really want is for the fallbacks to be flushed at the *end` of the `act` scope. Not within it. This only affects the noop and test renderer versions of `act`, which are implemented inside the reconciler. Whereas `ReactTestUtils.act` is implemented in "userspace" for backwards compatibility. This is fine because we didn't have any DOM Suspense tests that relied on this flag; they all use test renderer or noop. In the future, we'll probably want to move always use the reconciler implementation of `act`. It will not affect the prod bundle, because we currently only plan to support `act` in dev. Though we still haven't completely figured that out. However, regardless of whether we support a production `act` for users, we'll still need to write internal React tests in production mode. For that use case, we'll likely add our own internal version of `act` that assumes a mock Scheduler and might rely on hacks that don't 100% align up with the public one.
facebook-github-bot
added
CLA Signed
React Core Team
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 3a12db4:
|
acdlite
commented
Apr 14, 2020
Comment on lines
+3205
to
+3245
| if (flushMockScheduler !== undefined) { | ||
| const prevIsFlushing = isFlushingAct; | ||
| isFlushingAct = true; | ||
| try { | ||
| return flushMockScheduler(); | ||
| } finally { | ||
| isFlushingAct = prevIsFlushing; | ||
| } | ||
| } else { | ||
| // No mock scheduler available. However, the only type of pending work is | ||
| // passive effects, which we control. So we can flush that. | ||
| const prevIsFlushing = isFlushingAct; | ||
| isFlushingAct = true; | ||
| try { | ||
| let didFlushWork = false; | ||
| while (flushPassiveEffects()) { | ||
| didFlushWork = true; | ||
| } | ||
| return didFlushWork; | ||
| } finally { | ||
| isFlushingAct = prevIsFlushing; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is the relevant change. I set isFlushingAct while the work is being flushed at the end. Then I check this where we used to check the "force fallback" feature flag.
Details of bundled changes.Comparing: f3f3d77...3a12db4 react-art
react-test-renderer
react-dom
react-native-renderer
react-reconciler
ReactDOM: size: 0.0%, gzip: -0.0% Size changes (experimental) |
lunaruan
approved these changes
Apr 14, 2020
Details of bundled changes.Comparing: f3f3d77...3a12db4 react-art
react-test-renderer
react-native-renderer
react-dom
react-reconciler
ReactDOM: size: 0.0%, gzip: -0.0% Size changes (stable) |
acdlite
force-pushed
the
delete-flush-suspense-fallbacks-flag
branch
from
913b818
to
2d24402
Compare
|
@lunaruan One is built into the reconciler and relies on some internals. That's the implementation that's used by React Test Renderer and React Noop. There's another one implemented inside ReactTestUtils. It has to account for backwards compatibility with older versions of React. So it has lots of fallback mechanisms to try to work as best as possible for existing users. It's really only used to test components that include In Concurrent Mode, it goes further and flushes all pending React tasks, including rendering and suspended fallbacks. That can't be properly implemented without relying on internals. So they diverge slightly. Long term plan is to move everyone to the built-in implementation (the one I changed in this PR). |
acdlite
force-pushed
the
delete-flush-suspense-fallbacks-flag
branch
from
2d24402
to
7e27e5d
Compare
acdlite
commented
Apr 14, 2020
acdlite
force-pushed
the
delete-flush-suspense-fallbacks-flag
branch
5 times, most recently
from
c87f73e
to
3a12db4
Compare
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 was meant to be a temporary hack to unblock the
actwork, but it quickly spread throughout our tests.What it's meant to do is force fallbacks to flush inside
acteven in Concurrent Mode. It does this by wrapping thesetTimeoutcall in a check to see if it's in anactcontext. If so, it skips the delay and immediately commits the fallback.Really this is only meant for our internal React tests that need to incrementally render. Nobody outside our team (and Relay) needs to do that, yet. Even if/when we do support that, it may or may not be with the same
flushAndYieldpattern we use internally.However, even for our internal purposes, the behavior isn't right because a really common reason we flush work incrementally is to make assertions on the "suspended" state, before the fallback has committed. There's no way to do that from inside
actwith the behavior of this flag, because it causes the fallback to immediately commit. This has led us to not useactin a lot of our tests, or to write code that doesn't match what would actually happen in a real environment.What we really want is for the fallbacks to be flushed at the end of the
actscope. Not within it.This only affects the noop and test renderer versions of
act, which are implemented inside the reconciler. WhereasReactTestUtils.actis implemented in "userspace" for backwards compatibility. This is fine because we didn't have any DOM Suspense tests that relied on this flag; they all use test renderer or noop.In the future, we'll probably want to move always use the reconciler implementation of
act. It will not affect the prod bundle, because we currently only plan to supportactin dev. Though we still haven't completely figured that out. However, regardless of whether we support a productionactfor users, we'll still need to write internal React tests in production mode. For that use case, we'll likely add our own internal version ofactthat assumes a mock Scheduler and might rely on hacks that don't 100% align up with the public one.