Split recent passive effects changes into 2 flags #18030
Merged
Conversation
This file contains hidden or 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
Separate flags can now be used to opt passive effects into: 1) Deferring destroy functions on unmount to subsequent passive effects flush 2) Running all destroy functions (for all fibers) before create functions This allows us to test the less risky feature (2) separately from the more risky one.
bvaughn
force-pushed
the
split-passive-effect-flags
branch
from
6d0aace to
15c91fc
Compare
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 5897bd4:
|
bvaughn
commented
Feb 12, 2020
packages/shared/ReactFeatureFlags.js
Outdated
| // Controls behavior of deferred effect destroy functions during unmount. | ||
| // Previously these functions were run during commit (along with layout effects). | ||
| // Ideally we should delay these until after commit for performance reasons. | ||
| // This flag provides a killswitch if that proves to break existing code somehow. | ||
| // | ||
| // WARNING Only enable this flag in conjunction with runAllPassiveEffectDestroysBeforeCreates. |
There was a problem hiding this comment.
This constraint is a little gross
Unit tests failures in ReactHooksWithNoopRenderer and ReactSuspenseWithNoopRenderer will prevent us from accidentally using an invalid combination.
There was a problem hiding this comment.
Instead of two booleans you could use an enum, to prevent the invalid combination
There was a problem hiding this comment.
True. I considered that but our precedent has always been booleans for flags.
I don't feel strongly about this though.
There was a problem hiding this comment.
Alternative, in the code, you could also check for both booleans before you enable the "defer unmounts" feature.
Just want to make it harder for someone to accidentally toggle the wrong combination of GKs.
There was a problem hiding this comment.
Yeah, I hear you. That seems reasonable.
Details of bundled changes.Comparing: 58b8797...5897bd4 react
react-art
react-dom
scheduler
react-reconciler
Size changes (stable) |
Details of bundled changes.Comparing: 58b8797...5897bd4 react
react-dom
react-reconciler
scheduler
react-art
Size changes (experimental) |
…eEffectDestroysBeforeCreates is true
ermanzohre
approved these changes
Feb 18, 2020
threepointone
approved these changes
Feb 18, 2020
| act = ReactNoop.act; | ||
| } | ||
|
|
||
| [true, false].forEach(deferPassiveEffectCleanupDuringUnmount => { |
There was a problem hiding this comment.
wow function
much pure
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.
Separate flags can now be used to opt passive effects into:
deferPassiveEffectCleanupDuringUnmount: Defer passive effect destroy functions on unmount to subsequent passive effects flush. (FlushuseEffectclean up functions in the passive effects phase #17925)runAllPassiveEffectDestroysBeforeCreates: Running all passive effect destroy functions (for all fibers) before create functions. (Flush all passive destroy fns before calling create fns #17947)This allows us to test the less risky feature (#17947) separately from the more risky one.
Note that:
deferPassiveEffectCleanupDuringUnmountis ignored unlessrunAllPassiveEffectDestroysBeforeCreatesis also enabled.I've updated a couple of tests that were impacted by the change to explicitly test with both feature flag values so we don't have blind spots.
Be sure to use the
?w=1param to see the significant lines changed (+393 −99rather than+5,471 −5,177).