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

Delete flushSuspenseFallbacksInTests flag #18596

Merged
merged 2 commits into from
Apr 14, 2020

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Apr 14, 2020

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.

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 facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 14, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 14, 2020

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:

Sandbox Source
heuristic-tree-tlqyn Configuration

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;
}
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

@sizebot
Copy link

sizebot commented Apr 14, 2020

Details of bundled changes.

Comparing: f3f3d77...3a12db4

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js 0.0% +0.1% 658.21 KB 658.54 KB 138.43 KB 138.51 KB UMD_DEV
react-art.production.min.js 0.0% -0.0% 110.46 KB 110.46 KB 33.43 KB 33.43 KB UMD_PROD
react-art.development.js +0.1% +0.1% 561.52 KB 561.84 KB 120.89 KB 120.97 KB NODE_DEV
react-art.production.min.js 0.0% -0.0% 75.41 KB 75.41 KB 22.66 KB 22.66 KB NODE_PROD
ReactART-dev.js +0.1% +0.1% 595.9 KB 596.24 KB 125.4 KB 125.47 KB FB_WWW_DEV

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactTestRenderer-dev.js +0.2% +0.3% 577.38 KB 578.69 KB 121.84 KB 122.24 KB FB_WWW_DEV
react-test-renderer-shallow.development.js 0.0% -0.0% 38.72 KB 38.72 KB 9.42 KB 9.42 KB UMD_DEV
react-test-renderer-shallow.production.min.js 0.0% -0.0% 12.74 KB 12.74 KB 3.97 KB 3.97 KB UMD_PROD
react-test-renderer.development.js +0.2% +0.3% 569.56 KB 570.94 KB 118.49 KB 118.86 KB UMD_DEV
react-test-renderer.production.min.js -0.0% 🔺+0.1% 74.23 KB 74.22 KB 22.59 KB 22.6 KB UMD_PROD
react-test-renderer.development.js +0.2% +0.3% 543.08 KB 544.4 KB 117.13 KB 117.5 KB NODE_DEV
react-test-renderer.production.min.js -0.0% 🔺+0.1% 74.03 KB 74.02 KB 22.28 KB 22.3 KB NODE_PROD

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom-server.browser.development.js 0.0% -0.0% 162.2 KB 162.2 KB 41.21 KB 41.21 KB UMD_DEV
react-dom-server.browser.production.min.js 0.0% -0.0% 23.39 KB 23.39 KB 8.6 KB 8.6 KB UMD_PROD
react-dom.profiling.min.js 0.0% -0.0% 128.53 KB 128.53 KB 40.19 KB 40.19 KB NODE_PROFILING
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 4.91 KB 4.91 KB 1.64 KB 1.64 KB UMD_DEV
ReactDOM-dev.js +0.1% +0.1% 989.54 KB 990.13 KB 219.94 KB 220.13 KB FB_WWW_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.1% 1.2 KB 1.2 KB 705 B 704 B UMD_PROD
ReactDOM-profiling.js 0.0% -0.0% 424.08 KB 424.08 KB 76.81 KB 76.81 KB FB_WWW_PROFILING
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 4.43 KB 4.43 KB 1.55 KB 1.54 KB NODE_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.2% 1.02 KB 1.02 KB 617 B 616 B NODE_PROD
react-dom-test-utils.development.js 0.0% -0.0% 75.14 KB 75.14 KB 20.11 KB 20.11 KB UMD_DEV
react-dom-test-utils.production.min.js 0.0% -0.0% 13.23 KB 13.23 KB 4.9 KB 4.9 KB UMD_PROD
ReactDOMTesting-dev.js +0.1% +0.2% 931.63 KB 932.91 KB 207.58 KB 208.04 KB FB_WWW_DEV
ReactDOMTesting-prod.js 0.0% 0.0% 393.72 KB 393.76 KB 71.81 KB 71.81 KB FB_WWW_PROD
react-dom-test-utils.development.js 0.0% -0.0% 69.98 KB 69.98 KB 19.61 KB 19.61 KB NODE_DEV
ReactDOMTesting-profiling.js 0.0% 0.0% 393.72 KB 393.76 KB 71.81 KB 71.81 KB FB_WWW_PROFILING
react-dom-server.node.development.js 0.0% -0.0% 153.9 KB 153.9 KB 40.69 KB 40.69 KB NODE_DEV
react-dom-server.node.production.min.js 0.0% -0.0% 23.69 KB 23.69 KB 8.72 KB 8.72 KB NODE_PROD
react-dom-server.browser.production.min.js 0.0% -0.0% 23.28 KB 23.28 KB 8.57 KB 8.56 KB NODE_PROD
react-dom.development.js +0.1% +0.1% 940.9 KB 941.5 KB 205.6 KB 205.81 KB UMD_DEV
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 56.11 KB 56.11 KB 13.85 KB 13.85 KB UMD_DEV
react-dom.production.min.js 0.0% -0.0% 124.33 KB 124.33 KB 39.71 KB 39.71 KB UMD_PROD
ReactDOMForked-dev.js +0.1% +0.1% 989.56 KB 990.15 KB 220.08 KB 220.28 KB FB_WWW_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.0% 10 KB 10 KB 3.36 KB 3.36 KB UMD_PROD
ReactDOMServer-dev.js 0.0% -0.0% 161.46 KB 161.46 KB 40.99 KB 40.98 KB FB_WWW_DEV
react-dom.profiling.min.js 0.0% -0.0% 128.18 KB 128.18 KB 40.93 KB 40.93 KB UMD_PROFILING
ReactDOMForked-prod.js 0.0% -0.0% 413.75 KB 413.75 KB 75.04 KB 75.04 KB FB_WWW_PROD
react-dom.development.js +0.1% +0.1% 895.88 KB 896.46 KB 203.11 KB 203.32 KB NODE_DEV
ReactDOMForked-profiling.js 0.0% -0.0% 424.65 KB 424.65 KB 76.91 KB 76.91 KB FB_WWW_PROFILING
react-dom-unstable-fizz.node.development.js 0.0% -0.1% 5.18 KB 5.18 KB 1.7 KB 1.7 KB NODE_DEV
react-dom.production.min.js 0.0% -0.0% 124.53 KB 124.53 KB 38.95 KB 38.95 KB NODE_PROD
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.0% 9.75 KB 9.75 KB 3.25 KB 3.25 KB NODE_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-prod.js 0.0% -0.0% 271.2 KB 271.2 KB 46.69 KB 46.69 KB RN_OSS_PROD
ReactFabric-prod.js 0.0% -0.0% 263.19 KB 263.19 KB 45.18 KB 45.18 KB RN_OSS_PROD
ReactFabric-profiling.js 0.0% -0.0% 274.59 KB 274.59 KB 47.37 KB 47.37 KB RN_OSS_PROFILING
ReactFabric-dev.js +0.1% 0.0% 641.47 KB 641.81 KB 136.99 KB 137.05 KB RN_FB_DEV
ReactFabric-prod.js 0.0% -0.0% 263.34 KB 263.34 KB 45.21 KB 45.21 KB RN_FB_PROD
ReactNativeRenderer-dev.js +0.1% +0.1% 654.95 KB 655.34 KB 140.65 KB 140.74 KB RN_OSS_DEV
ReactNativeRenderer-dev.js +0.1% +0.1% 659.75 KB 660.14 KB 141.45 KB 141.54 KB RN_FB_DEV
ReactFabric-dev.js +0.1% +0.1% 636.66 KB 637 KB 136.21 KB 136.28 KB RN_OSS_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler-reflection.development.js 0.0% -0.0% 16.23 KB 16.23 KB 4.92 KB 4.92 KB NODE_DEV
react-reconciler-reflection.production.min.js 0.0% -0.1% 2.58 KB 2.58 KB 1.09 KB 1.09 KB NODE_PROD
react-reconciler.development.js +0.2% +0.3% 603.25 KB 604.57 KB 127.55 KB 127.94 KB NODE_DEV
react-reconciler.production.min.js -0.0% 🔺+0.1% 80.82 KB 80.81 KB 23.91 KB 23.93 KB NODE_PROD

ReactDOM: size: 0.0%, gzip: -0.0%

Size changes (experimental)

Generated by 🚫 dangerJS against 3a12db4

Copy link
Contributor

@lunaruan lunaruan left a comment

Choose a reason for hiding this comment

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

Out of curiosity, why do we have 3 different versions of act?

@sizebot
Copy link

sizebot commented Apr 14, 2020

Details of bundled changes.

Comparing: f3f3d77...3a12db4

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.1% +0.1% 634.08 KB 634.41 KB 133.64 KB 133.71 KB UMD_DEV
react-art.production.min.js 0.0% -0.0% 107.18 KB 107.18 KB 32.51 KB 32.5 KB UMD_PROD
react-art.development.js +0.1% +0.1% 538.39 KB 538.71 KB 116.06 KB 116.14 KB NODE_DEV
react-art.production.min.js 0.0% -0.0% 72.18 KB 72.18 KB 21.69 KB 21.69 KB NODE_PROD
ReactART-dev.js +0.1% +0.1% 605.96 KB 606.31 KB 127.41 KB 127.48 KB FB_WWW_DEV
ReactART-prod.js 0.0% -0.0% 251.11 KB 251.11 KB 42.66 KB 42.66 KB FB_WWW_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.2% +0.3% 569.53 KB 570.92 KB 118.47 KB 118.85 KB UMD_DEV
react-test-renderer.production.min.js -0.0% 🔺+0.1% 74.2 KB 74.2 KB 22.57 KB 22.58 KB UMD_PROD
react-test-renderer-shallow.development.js 0.0% -0.0% 38.71 KB 38.71 KB 9.42 KB 9.42 KB UMD_DEV
react-test-renderer-shallow.production.min.js 0.0% -0.0% 12.73 KB 12.73 KB 3.96 KB 3.96 KB UMD_PROD
react-test-renderer.development.js +0.2% +0.3% 543.05 KB 544.37 KB 117.12 KB 117.49 KB NODE_DEV
react-test-renderer.production.min.js -0.0% 🔺+0.1% 74 KB 74 KB 22.27 KB 22.28 KB NODE_PROD
ReactTestRenderer-dev.js +0.2% +0.3% 577.37 KB 578.68 KB 121.83 KB 122.24 KB FB_WWW_DEV

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.1% +0.1% 654.93 KB 655.33 KB 140.64 KB 140.73 KB RN_OSS_DEV
ReactNativeRenderer-profiling.js 0.0% -0.0% 282.62 KB 282.62 KB 48.87 KB 48.87 KB RN_OSS_PROFILING
ReactFabric-dev.js +0.1% +0.1% 636.64 KB 636.99 KB 136.2 KB 136.28 KB RN_OSS_DEV
ReactFabric-prod.js 0.0% -0.0% 263.17 KB 263.17 KB 45.17 KB 45.17 KB RN_OSS_PROD

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.production.min.js 0.0% -0.0% 119.7 KB 119.7 KB 37.54 KB 37.54 KB NODE_PROD
react-dom-test-utils.development.js 0.0% -0.0% 75.12 KB 75.12 KB 20.11 KB 20.1 KB UMD_DEV
ReactDOMTesting-profiling.js 0.0% 0.0% 405.55 KB 405.58 KB 73.59 KB 73.6 KB FB_WWW_PROFILING
react-dom-server.browser.production.min.js 0.0% -0.0% 22.82 KB 22.82 KB 8.48 KB 8.48 KB NODE_PROD
react-dom.profiling.min.js 0.0% -0.0% 123.58 KB 123.58 KB 38.67 KB 38.67 KB NODE_PROFILING
react-dom-test-utils.production.min.js 0.0% -0.0% 13.22 KB 13.22 KB 4.89 KB 4.89 KB UMD_PROD
react-dom-server.node.development.js 0.0% -0.0% 147.14 KB 147.14 KB 38.93 KB 38.93 KB NODE_DEV
react-dom-server.node.production.min.js 0.0% -0.0% 23.23 KB 23.23 KB 8.65 KB 8.65 KB NODE_PROD
ReactDOMForked-dev.js +0.1% +0.1% 1014.82 KB 1015.41 KB 225.33 KB 225.53 KB FB_WWW_DEV
ReactDOMForked-prod.js 0.0% -0.0% 425.18 KB 425.18 KB 76.69 KB 76.69 KB FB_WWW_PROD
react-dom-unstable-fizz.node.development.js 0.0% -0.1% 5.16 KB 5.16 KB 1.69 KB 1.69 KB NODE_DEV
react-dom.development.js +0.1% +0.1% 907.21 KB 907.82 KB 199.15 KB 199.36 KB UMD_DEV
react-dom-server.browser.development.js 0.0% -0.0% 155.09 KB 155.09 KB 39.45 KB 39.44 KB UMD_DEV
react-dom-unstable-fizz.node.production.min.js 0.0% 🔺+0.2% 1.16 KB 1.16 KB 658 B 659 B NODE_PROD
react-dom.production.min.js 0.0% -0.0% 119.58 KB 119.58 KB 38.26 KB 38.26 KB UMD_PROD
react-dom-server.browser.production.min.js 0.0% -0.0% 22.92 KB 22.92 KB 8.51 KB 8.51 KB UMD_PROD
ReactDOMTesting-dev.js +0.1% +0.2% 958.17 KB 959.46 KB 213.22 KB 213.67 KB FB_WWW_DEV
react-dom.development.js +0.1% +0.1% 863.63 KB 864.21 KB 196.72 KB 196.91 KB NODE_DEV
ReactDOMTesting-prod.js 0.0% 0.0% 405.55 KB 405.58 KB 73.59 KB 73.6 KB FB_WWW_PROD
react-dom-server.browser.development.js 0.0% -0.0% 145.92 KB 145.92 KB 38.68 KB 38.68 KB NODE_DEV
ReactDOM-dev.js +0.1% +0.1% 1014.81 KB 1015.39 KB 225.18 KB 225.38 KB FB_WWW_DEV
ReactDOM-prod.js 0.0% -0.0% 424.6 KB 424.6 KB 76.6 KB 76.6 KB FB_WWW_PROD
react-dom-test-utils.development.js 0.0% -0.0% 69.97 KB 69.97 KB 19.6 KB 19.6 KB NODE_DEV
ReactDOMServer-prod.js 0.0% -0.0% 53.3 KB 53.3 KB 12.85 KB 12.85 KB FB_WWW_PROD
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 4.42 KB 4.42 KB 1.54 KB 1.54 KB NODE_DEV
ReactDOM-profiling.js 0.0% -0.0% 435.56 KB 435.56 KB 78.51 KB 78.5 KB FB_WWW_PROFILING
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 56.1 KB 56.1 KB 13.84 KB 13.84 KB UMD_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.2% 1 KB 1 KB 609 B 608 B NODE_PROD
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 53.2 KB 53.2 KB 13.64 KB 13.64 KB NODE_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.0% 9.73 KB 9.73 KB 3.25 KB 3.25 KB NODE_PROD

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.2% +0.3% 577.75 KB 579.07 KB 122.22 KB 122.61 KB NODE_DEV
react-reconciler-reflection.development.js 0.0% -0.0% 16.22 KB 16.22 KB 4.91 KB 4.91 KB NODE_DEV
react-reconciler.production.min.js -0.0% 0.0% 77.17 KB 77.16 KB 22.88 KB 22.89 KB NODE_PROD
react-reconciler-reflection.production.min.js 0.0% -0.2% 2.57 KB 2.57 KB 1.08 KB 1.08 KB NODE_PROD

ReactDOM: size: 0.0%, gzip: -0.0%

Size changes (stable)

Generated by 🚫 dangerJS against 3a12db4

@acdlite acdlite force-pushed the delete-flush-suspense-fallbacks-flag branch from 913b818 to 2d24402 Compare April 14, 2020 01:13
@acdlite
Copy link
Collaborator Author

acdlite commented Apr 14, 2020

@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 useEffect, because those effects are "passive" — unlike everything else in React's legacy mode, they are scheduled in an async task. Tests need to flush those tasks before making assertions. So that's what act does: at the end of the act body, it flushes all the passive effects.

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 acdlite force-pushed the delete-flush-suspense-fallbacks-flag branch from 2d24402 to 7e27e5d Compare April 14, 2020 01:25
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