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

Flush useEffect clean up functions in the passive effects phase #17925

Merged

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Jan 29, 2020

This PR cleans up our hook effect tags and also fixes the case where we were too eagerly flushing passive effect destroy functions on unmount. (As a side effect, we are also now reliably able to distinguish between passive and layout effects during unmount, which will improve the new Profiler methods added in #17910.)

The two major changes in this PR are:

  • 9a38b12: Defer passive destroy functions during an unmount until other passive effects are flushed.
  • 1c6be34: Cleanup effect tags.

This is a change in behavior/timing for destroy functions so it may cause code to break unexpectedly. For this reason, it has been added behind a killswitch feature flag (deferPassiveEffectCleanupDuringUnmount).

I've also identified some follow up work for passive effects, but to keep this PR small and easy to review, I've just added a TODO for this and will be tracking it with a separate issue, #17945.

@facebook-github-bot facebook-github-bot added React Core Team Opened by a member of the React Core Team CLA Signed labels Jan 29, 2020
}
effect = effect.next;
} while (effect !== firstEffect);
});
} else {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the old code path, in case we enable the kill switch.

// Note: We currently never use MountMutation, but useLayoutEffect uses UnmountMutation.
// This is to ensure ALL destroy fns are run before create fns,
// without requiring us to traverse the effects list an extra time during commit.
// This sequence prevents sibling destroy and create fns from interfering with each other.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added these comments for myself, since it wasn't clear to me why we were using these tags.

export const MountLayout = /* */ 0b000100000;
export const MountPassive = /* */ 0b001000000;
export const UnmountPassive = /* */ 0b010000000;
export const NoEffectPassiveUnmountFiber = /**/ 0b100000000;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name of the new effect tag I added, NoEffectPassiveUnmountFiber, is kind of stupid. I'd welcome a solution for a better name. 🤪

Copy link
Collaborator

@acdlite acdlite Jan 30, 2020

Choose a reason for hiding this comment

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

I think it should be possible to simplify these tags by removing the Mount/Unmount . I think that's already handled because commitHookEffectList accepts two separate arguments for which tags to mount and which tags to unmount.

Would it work if the only tags we had were this?

// Represents whether effect should fire.
export const HasEffect = /*  */ 0b0001;
// Represents the phase in which the effect (not the clean-up) fires.
export const Snapshot = /*   */ 0b0010;
export const Layout = /*     */ 0b0100;
export const Passive = /*    */ 0b1000;

So during an update you would fire the clean up of any hook that has (for passive effects) that has Passive | HasEffect. When the fiber is deleted, you would only check Passive.

Copy link
Contributor Author

@bvaughn bvaughn Jan 31, 2020

Choose a reason for hiding this comment

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

I think this cleanup suggestion makes sense, although while looking into it I realized we have a potential bug with the sequence/timing of passive effects. (We don't call all destroy functions before create functions.) I'll take a pass at the effect tag cleanup and separating the passive effect destroy/create phases too. If it starts looking like that might be a bigger change though, I may split it out into a follow up PR (#17945)

@bvaughn bvaughn force-pushed the defer-passive-effect-cleanup-during-unmount branch from bf7b3c3 to 50f1706 Compare January 29, 2020 19:28
function updateEffectImpl(
fiberEffectTag,
hookEffectTag,
noWorkUnmountFiberHookEffectTag,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This name isn't the greatest either. I welcome better suggestions.

The purpose of the changes to this function is to retain the ability to distinguish between layout and passive effects even if a render has no dependency changes. Previously, we would not be able to distinguish between the two during unmount.

@codesandbox-ci
Copy link

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 bf7b3c3:

Sandbox Source
clever-curran-z9cbb Configuration

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 29, 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 606af56:

Sandbox Source
shy-snowflake-1oyz6 Configuration

@sizebot
Copy link

sizebot commented Jan 29, 2020

Details of bundled changes.

Comparing: 434770c...606af56

react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react.development.js +0.3% +0.4% 114 KB 114.31 KB 29.14 KB 29.26 KB UMD_DEV
React-prod.js 0.0% -0.0% 17.91 KB 17.91 KB 4.68 KB 4.68 KB FB_WWW_PROD
React-profiling.js 0.0% -0.0% 17.91 KB 17.91 KB 4.68 KB 4.68 KB FB_WWW_PROFILING
react.development.js +0.4% +0.6% 73.52 KB 73.83 KB 19.48 KB 19.6 KB NODE_DEV
react.production.min.js 0.0% -0.0% 6.96 KB 6.96 KB 2.86 KB 2.86 KB NODE_PROD
React-dev.js +0.1% +0.1% 73.63 KB 73.72 KB 18.74 KB 18.76 KB FB_WWW_DEV

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.profiling.min.js +0.1% -0.0% 123.69 KB 123.78 KB 38.79 KB 38.77 KB NODE_PROFILING
react-dom-server.browser.development.js +0.2% +0.3% 138.97 KB 139.28 KB 36.81 KB 36.93 KB UMD_DEV
react-dom-server.browser.production.min.js 0.0% -0.0% 20.46 KB 20.46 KB 7.5 KB 7.5 KB UMD_PROD
react-dom-test-utils.development.js +0.6% +0.8% 54.86 KB 55.17 KB 15.43 KB 15.55 KB UMD_DEV
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 3.88 KB 3.88 KB 1.55 KB 1.55 KB UMD_DEV
react-dom-test-utils.production.min.js 0.0% -0.1% 11.18 KB 11.18 KB 4.15 KB 4.15 KB UMD_PROD
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.1% 1.21 KB 1.21 KB 711 B 710 B UMD_PROD
react-dom-test-utils.development.js +0.6% +0.8% 53.13 KB 53.44 KB 15.11 KB 15.23 KB NODE_DEV
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 3.71 KB 3.71 KB 1.5 KB 1.5 KB NODE_DEV
react-dom-test-utils.production.min.js 0.0% -0.1% 10.95 KB 10.95 KB 4.09 KB 4.09 KB NODE_PROD
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.2% 1.05 KB 1.05 KB 642 B 641 B NODE_PROD
react-dom.development.js +0.3% +0.3% 961.05 KB 964.2 KB 215.77 KB 216.5 KB UMD_DEV
react-dom.production.min.js 🔺+0.1% -0.0% 119.81 KB 119.9 KB 38.43 KB 38.42 KB UMD_PROD
react-dom.profiling.min.js +0.1% -0.0% 123.45 KB 123.54 KB 39.58 KB 39.56 KB UMD_PROFILING
react-dom.development.js +0.3% +0.3% 955.12 KB 958.27 KB 214.09 KB 214.81 KB NODE_DEV
react-dom-server.node.development.js +0.2% +0.3% 136.01 KB 136.32 KB 36.02 KB 36.13 KB NODE_DEV
react-dom.production.min.js 🔺+0.1% -0.0% 119.91 KB 120 KB 37.67 KB 37.66 KB NODE_PROD
ReactTestUtils-dev.js +0.2% +0.2% 52.29 KB 52.38 KB 14.27 KB 14.29 KB FB_WWW_DEV
react-dom-server.browser.development.js +0.2% +0.3% 134.9 KB 135.21 KB 35.79 KB 35.9 KB NODE_DEV
ReactDOM-dev.js +0.3% +0.3% 982.87 KB 985.7 KB 216.94 KB 217.53 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.4% 🔺+0.4% 392.26 KB 393.64 KB 71.63 KB 71.92 KB FB_WWW_PROD
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 60.91 KB 60.91 KB 16.07 KB 16.07 KB UMD_DEV
ReactDOM-profiling.js +0.4% +0.4% 403.47 KB 404.9 KB 73.76 KB 74.08 KB FB_WWW_PROFILING
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 60.61 KB 60.61 KB 16 KB 16 KB NODE_DEV
ReactDOMServer-dev.js +0.1% +0.1% 140.24 KB 140.33 KB 35.49 KB 35.53 KB FB_WWW_DEV
react-dom-unstable-fizz.node.production.min.js 0.0% -0.1% 1.21 KB 1.21 KB 698 B 697 B NODE_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.5% +0.5% 623.23 KB 626.07 KB 131.53 KB 132.13 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.1% 🔺+0.2% 71.93 KB 72.02 KB 21.94 KB 21.98 KB UMD_PROD
react-test-renderer-shallow.development.js 0.0% -0.0% 37.89 KB 37.89 KB 9.84 KB 9.84 KB UMD_DEV
react-test-renderer-shallow.production.min.js 0.0% -0.0% 11.79 KB 11.79 KB 3.71 KB 3.71 KB NODE_PROD
react-test-renderer.development.js +0.5% +0.5% 618.49 KB 621.34 KB 130.34 KB 130.94 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.1% -0.0% 71.63 KB 71.72 KB 21.56 KB 21.56 KB NODE_PROD
ReactTestRenderer-dev.js +0.4% +0.5% 634.34 KB 637.13 KB 131.17 KB 131.77 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.4% +0.4% 751.29 KB 754.08 KB 157.96 KB 158.55 KB RN_OSS_DEV
ReactFabric-dev.js +0.4% +0.4% 741.9 KB 744.69 KB 155.75 KB 156.34 KB RN_OSS_DEV
ReactNativeRenderer-dev.js +0.4% +0.4% 751.46 KB 754.25 KB 158.05 KB 158.65 KB RN_FB_DEV
ReactFabric-prod.js 🔺+0.1% -0.0% 266.76 KB 267.06 KB 45.76 KB 45.74 KB RN_OSS_PROD
ReactNativeRenderer-prod.js 🔺+0.1% -0.0% 274.72 KB 275.03 KB 47.08 KB 47.08 KB RN_FB_PROD
ReactFabric-profiling.js +0.1% -0.0% 277.92 KB 278.22 KB 47.89 KB 47.87 KB RN_OSS_PROFILING
ReactNativeRenderer-profiling.js +0.1% -0.0% 285.92 KB 286.23 KB 49.23 KB 49.22 KB RN_FB_PROFILING
ReactNativeRenderer-prod.js 🔺+0.1% -0.0% 274.33 KB 274.64 KB 47.01 KB 47 KB RN_OSS_PROD
ReactFabric-dev.js +0.4% +0.4% 742.09 KB 744.88 KB 155.84 KB 156.43 KB RN_FB_DEV
ReactNativeRenderer-profiling.js +0.1% -0.0% 285.54 KB 285.84 KB 49.16 KB 49.15 KB RN_OSS_PROFILING
ReactFabric-prod.js 🔺+0.1% -0.0% 267.11 KB 267.41 KB 45.83 KB 45.81 KB RN_FB_PROD
ReactFabric-profiling.js +0.1% -0.0% 278.26 KB 278.56 KB 47.95 KB 47.93 KB RN_FB_PROFILING

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactART-dev.js +0.5% +0.5% 622.66 KB 625.5 KB 128.6 KB 129.21 KB FB_WWW_DEV
ReactART-prod.js 🔺+0.7% 🔺+0.8% 236.23 KB 237.97 KB 39.78 KB 40.11 KB FB_WWW_PROD
react-art.development.js +0.5% +0.5% 678.52 KB 681.67 KB 146.01 KB 146.73 KB UMD_DEV
react-art.production.min.js 🔺+0.1% 0.0% 107.32 KB 107.41 KB 32.52 KB 32.53 KB UMD_PROD
react-art.development.js +0.5% +0.6% 609.2 KB 612.35 KB 128.59 KB 129.31 KB NODE_DEV
react-art.production.min.js 🔺+0.1% -0.0% 72.29 KB 72.38 KB 21.68 KB 21.68 KB NODE_PROD

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler-persistent.development.js +0.5% +0.5% 607.33 KB 610.17 KB 126.56 KB 127.16 KB NODE_DEV
react-reconciler-reflection.development.js +1.6% +2.0% 19.59 KB 19.9 KB 6.42 KB 6.55 KB NODE_DEV
react-reconciler-persistent.production.min.js 🔺+0.1% -0.1% 72.53 KB 72.62 KB 21.32 KB 21.31 KB NODE_PROD
react-reconciler-reflection.production.min.js 0.0% -0.1% 2.86 KB 2.86 KB 1.24 KB 1.24 KB NODE_PROD
react-reconciler.development.js +0.5% +0.6% 610.2 KB 613.35 KB 127.76 KB 128.48 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.1% -0.0% 75.29 KB 75.38 KB 21.96 KB 21.96 KB NODE_PROD

scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
scheduler-tracing.development.js +2.7% +4.7% 11.62 KB 11.93 KB 2.94 KB 3.08 KB NODE_DEV
scheduler-tracing.production.min.js 0.0% -0.5% 741 B 741 B 393 B 391 B NODE_PROD
scheduler-unstable_mock.development.js 0.0% 0.0% 22.72 KB 22.72 KB 5.42 KB 5.42 KB UMD_DEV
scheduler-unstable_mock.production.min.js 0.0% 0.0% 4.73 KB 4.73 KB 1.98 KB 1.98 KB UMD_PROD
scheduler-unstable_mock.production.min.js 0.0% 🔺+0.1% 4.72 KB 4.72 KB 1.92 KB 1.92 KB NODE_PROD
SchedulerTracing-dev.js +1.0% +1.3% 9.75 KB 9.84 KB 2.09 KB 2.11 KB FB_WWW_DEV

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

Size changes (experimental)

Generated by 🚫 dangerJS against 606af56

@sizebot
Copy link

sizebot commented Jan 29, 2020

Details of bundled changes.

Comparing: 434770c...606af56

react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react.development.js +0.3% +0.4% 113.98 KB 114.29 KB 29.13 KB 29.25 KB UMD_DEV
react.profiling.min.js 0.0% -0.0% 15.85 KB 15.85 KB 5.95 KB 5.95 KB UMD_PROFILING
react.development.js +0.4% +0.6% 73.5 KB 73.8 KB 19.47 KB 19.6 KB NODE_DEV
react.production.min.js 0.0% -0.0% 6.54 KB 6.54 KB 2.72 KB 2.72 KB NODE_PROD

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom-server.browser.production.min.js 0.0% -0.0% 19.93 KB 19.93 KB 7.39 KB 7.39 KB NODE_PROD
react-dom-test-utils.development.js +0.6% +0.8% 53.12 KB 53.43 KB 15.1 KB 15.23 KB NODE_DEV
react-dom.production.min.js 🔺+0.1% -0.0% 115.91 KB 116 KB 37.3 KB 37.28 KB UMD_PROD
react-dom.profiling.min.js +0.1% 0.0% 119.44 KB 119.53 KB 38.43 KB 38.44 KB UMD_PROFILING
react-dom.development.js +0.3% +0.3% 955.09 KB 958.24 KB 214.06 KB 214.79 KB NODE_DEV
react-dom.production.min.js 🔺+0.1% -0.0% 115.98 KB 116.07 KB 36.62 KB 36.62 KB NODE_PROD
react-dom.profiling.min.js +0.1% -0.0% 119.65 KB 119.74 KB 37.74 KB 37.73 KB NODE_PROFILING
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 3.87 KB 3.87 KB 1.54 KB 1.54 KB UMD_DEV
react-dom-server.browser.development.js +0.2% +0.3% 138.95 KB 139.25 KB 36.81 KB 36.93 KB UMD_DEV
react-dom-server.browser.production.min.js 0.0% -0.0% 20 KB 20 KB 7.4 KB 7.4 KB UMD_PROD
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 3.7 KB 3.7 KB 1.5 KB 1.49 KB NODE_DEV
react-dom-test-utils.development.js +0.6% +0.8% 54.84 KB 55.15 KB 15.43 KB 15.55 KB UMD_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.2% 1.04 KB 1.04 KB 634 B 633 B NODE_PROD
react-dom-test-utils.production.min.js 0.0% -0.1% 11.17 KB 11.17 KB 4.14 KB 4.14 KB UMD_PROD
react-dom-server.browser.development.js +0.2% +0.3% 134.88 KB 135.19 KB 35.78 KB 35.9 KB NODE_DEV
react-dom-test-utils.production.min.js 0.0% -0.1% 10.94 KB 10.94 KB 4.09 KB 4.08 KB NODE_PROD
react-dom-unstable-fizz.node.development.js 0.0% -0.1% 4.4 KB 4.4 KB 1.64 KB 1.64 KB NODE_DEV
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 60.89 KB 60.89 KB 16.07 KB 16.06 KB UMD_DEV
react-dom-server.node.development.js +0.2% +0.3% 135.99 KB 136.3 KB 36.02 KB 36.13 KB NODE_DEV
react-dom.development.js +0.3% +0.3% 961.03 KB 964.18 KB 215.75 KB 216.48 KB UMD_DEV
react-dom-server.node.production.min.js 0.0% -0.0% 20.34 KB 20.34 KB 7.54 KB 7.54 KB NODE_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactFabric-dev.js +0.4% +0.4% 741.89 KB 744.68 KB 155.75 KB 156.33 KB RN_OSS_DEV
ReactFabric-prod.js 🔺+0.1% -0.0% 266.74 KB 267.05 KB 45.75 KB 45.73 KB RN_OSS_PROD
ReactNativeRenderer-dev.js +0.4% +0.4% 751.27 KB 754.06 KB 157.95 KB 158.54 KB RN_OSS_DEV
ReactFabric-profiling.js +0.1% -0.0% 277.9 KB 278.21 KB 47.88 KB 47.86 KB RN_OSS_PROFILING
ReactNativeRenderer-prod.js 🔺+0.1% -0.0% 274.32 KB 274.62 KB 47 KB 47 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js +0.1% -0.0% 285.53 KB 285.83 KB 49.15 KB 49.14 KB RN_OSS_PROFILING

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler-persistent.production.min.js 🔺+0.1% -0.1% 72.52 KB 72.61 KB 21.31 KB 21.3 KB NODE_PROD
react-reconciler.development.js +0.5% +0.6% 610.19 KB 613.34 KB 127.76 KB 128.48 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.1% -0.1% 72.51 KB 72.6 KB 21.31 KB 21.29 KB NODE_PROD
react-reconciler-reflection.development.js +1.6% +2.0% 19.58 KB 19.89 KB 6.42 KB 6.55 KB NODE_DEV
react-reconciler-reflection.production.min.js 0.0% -0.1% 2.85 KB 2.85 KB 1.24 KB 1.24 KB NODE_PROD
react-reconciler-persistent.development.js +0.5% +0.5% 607.32 KB 610.16 KB 126.55 KB 127.16 KB NODE_DEV

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.production.min.js 🔺+0.1% 0.0% 104.77 KB 104.86 KB 31.83 KB 31.83 KB UMD_PROD
react-art.development.js +0.5% +0.5% 678.5 KB 681.64 KB 146.01 KB 146.73 KB UMD_DEV
react-art.development.js +0.5% +0.6% 609.17 KB 612.33 KB 128.59 KB 129.3 KB NODE_DEV
react-art.production.min.js 🔺+0.1% 🔺+0.1% 69.77 KB 69.86 KB 21 KB 21.01 KB NODE_PROD

scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
scheduler-unstable_mock.development.js 0.0% 0.0% 22.71 KB 22.71 KB 5.41 KB 5.41 KB UMD_DEV
scheduler-unstable_mock.production.min.js 0.0% 🔺+0.1% 4.71 KB 4.71 KB 1.91 KB 1.91 KB NODE_PROD
scheduler-tracing.development.js +2.7% +4.7% 11.61 KB 11.92 KB 2.94 KB 3.08 KB NODE_DEV
scheduler-tracing.production.min.js 0.0% 🔺+0.3% 728 B 728 B 382 B 383 B NODE_PROD
scheduler-tracing.profiling.min.js 0.0% -0.1% 3.25 KB 3.25 KB 993 B 992 B NODE_PROFILING

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer-shallow.production.min.js 0.0% -0.0% 11.64 KB 11.64 KB 3.59 KB 3.59 KB UMD_PROD
react-test-renderer-shallow.production.min.js 0.0% -0.0% 11.78 KB 11.78 KB 3.7 KB 3.7 KB NODE_PROD
react-test-renderer.development.js +0.5% +0.5% 623.2 KB 626.04 KB 131.52 KB 132.12 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.1% 🔺+0.2% 71.91 KB 72 KB 21.92 KB 21.96 KB UMD_PROD
react-test-renderer.development.js +0.5% +0.5% 618.47 KB 621.31 KB 130.33 KB 130.93 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.1% -0.0% 71.61 KB 71.7 KB 21.54 KB 21.54 KB NODE_PROD

ReactDOM: size: 🔺+0.1%, gzip: -0.0%

Size changes (stable)

Generated by 🚫 dangerJS against 606af56

// HACK
// This update is kind of gross since it exists to test an internal implementation detail:
// Effects without updating dependencies lose their layout/passive tag during an update.
// A special type of no-update tag (NoEffectPassiveUnmountFiber) is used to track these for later.
Copy link
Collaborator

Choose a reason for hiding this comment

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

To make it less "hacky" you could include the count in the log message. So that the effect actually depends on it.

Then make the other effect close over a different prop. count2 or whatever.

Then during the update, you change one of the props but not the other. Only that effect should fire; the other one will bail out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, sure. That"hacky" part is that I'm only doing this intermediate update because I know that used to cause the specific implementation to blur the passive and layout effects together.

if (deferPassiveEffectCleanupDuringUnmount) {
pendingUnmountedPassiveEffectDestroyFunctions.push(destroy);
rootDoesHavePassiveEffects = true;
scheduleCallback(NormalPriority, () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will schedule a callback for every clean-up effect. Need to check if rootDoesHavePassiveEffects is already true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good call!

@bvaughn bvaughn force-pushed the defer-passive-effect-cleanup-during-unmount branch from 50f1706 to d140ab0 Compare January 31, 2020 00:02
@bvaughn bvaughn requested a review from acdlite January 31, 2020 19:21
@bvaughn bvaughn force-pushed the defer-passive-effect-cleanup-during-unmount branch from d140ab0 to 1c6be34 Compare January 31, 2020 19:22
@@ -250,7 +248,6 @@ function commitBeforeMutationLifeCycles(
case ForwardRef:
case SimpleMemoComponent:
case Chunk: {
commitHookEffectList(UnmountSnapshot, NoHookEffect, finishedWork);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have a snapshot phase hook (yet) so this call was walking the effect list for no reason. I've removed it.

i++
) {
const destroy = pendingUnmountedPassiveEffectDestroyFunctions[i];
invokeGuardedCallback(null, destroy, null);
Copy link
Contributor Author

@bvaughn bvaughn Jan 31, 2020

Choose a reason for hiding this comment

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

This loop approach has an unintentional side effect of being "better" than our normal destroy approach, since an error thrown by one destroy function won't prevent other destroy functions on the same Fiber from being called.

Copy link
Collaborator

@acdlite acdlite Jan 31, 2020

Choose a reason for hiding this comment

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

Did you confirm that with a test? I would expect the current behavior to also unmount all the destroy functions on a fiber, even if one throws, because if one of them throws, then the nearest error boundary fiber gets deleted, which triggers commitNestedUnmounts, which then calls the remaining clean-up functions and wraps each call in a try catch:

safelyCallDestroy(current, destroy);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes I think you are right. Disregard!

// 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.
export const deferPassiveEffectCleanupDuringUnmount = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open question: should I set this to false for now (for open source releases)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah probably, just to be safe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

K, will do then.

Copy link
Collaborator

@acdlite acdlite left a comment

Choose a reason for hiding this comment

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

Left some nits. Looks good to me!

if ((effect.tag & mountTag) !== NoHookEffect) {
if (
(effect.tag & HookHasEffect) !== NoHookEffect &&
(effect.tag & mountTag) !== NoHookEffect
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: You can save a check here if you assume mountTag and unmountTag already include HookHasEffect:

(effect.tag & mountTag) === mountTag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm! But they specifically don't.

We use the tag to specify the type of effect even if there's no work currently required by the hook. This lets us properly differentiate between passive and layout effects in the unmount case. (NoHookEffect is used to signal that there's also work for that specific hook during the current commit.)

// before calling any create functions. The current approach only serializes
// these for a single fiber.
commitHookEffectList(HookPassive, NoHookEffect, finishedWork);
commitHookEffectList(NoHookEffect, HookPassive, finishedWork);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Following from previous comment, here you would combine the tag with HookHasEffect before passing them in:

commitHookEffectList(HookPassive | HookHasEffect, NoHookEffect, finishedWork);
commitHookEffectList(NoHookEffect, HookPassive | HookHasEffect, finishedWork);

(I assume Closure inlines those values, or you could also hoist them out to module scope)

Brian Vaughn added 5 commits February 3, 2020 10:44
This is a change in behavior that may cause broken product code, so it has been added behind a killswitch (deferPassiveEffectCleanupDuringUnmount)
Updated enqueuePendingPassiveEffectDestroyFn() to check rootDoesHavePassiveEffects before scheduling a new callback. This way we'll only schedule (at most) one.
We previously used separate Mount* and Unmount* tags to track hooks work for each phase (snapshot, mutation, layout, and passive). This was somewhat complicated to trace through and there were man tag types we never even used (e.g. UnmountLayout, MountMutation, UnmountSnapshot). In addition to this, it left passive and layout hooks looking the same after renders without changed dependencies, which meant we were unable to reliably defer passive effect destroy functions until after the commit phase.

This commit reduces the effect tag types to only include Layout and Passive and differentiates between work and no-work with an HasEffect flag.
notsidney added a commit to rowyio/rowy that referenced this pull request Jul 12, 2021
* develop:
  add badge to UserMenu when update is available
  migrate all links to WIKI_LINKS
  add UpdateChecker
  default signInOptions to google if signInOptions doc is missing
  fix TableSettings multiSelects
  TextEditor: fix values not saving in react 17 (thanks @oliviertassinari ) mui/material-ui#25560 (comment) facebook/react#17925
  expose dataset location option for BQ spark
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.

5 participants