Strict Mode: Reuse memoized result from first pass #25583
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
In Strict Mode, during development, user functions are double invoked to help detect side effects. Currently, the way we implement this is to completely discard the first pass and start over. Theoretically this should be fine because components are idempotent. However, it's a bit tricky to get right because our implementation (i.e. `renderWithHooks`) is not completely idemponent with respect to internal data structures, like the work-in-progress fiber. In the past we've had to be really careful to avoid subtle bugs — for example, during the initial mount, `setState` functions are bound to the particular hook instances that were created during that render. If we compute new hook instances, we must also compute new children, and they must correspond to each other. This commit addresses a similar issue that came up related to `use`: when something suspends, `use` reuses the promise that was passed during the first attempt. This is itself a form of memoization. We need to be able to memoize the reactive inputs to the `use` call using a hook (i.e. `useMemo`), which means, the reactive inputs to `use` must come from the same component invocation as the output. The solution I've chosen is, rather than double invoke the entire `renderWithHook` function, we should double invoke each individual user function. It's a bit confusing but here's how it works: We will invoke the entire component function twice. However, during the second invocation of the component, the hook state from the first invocation will be reused. That means things like `useMemo` functions won't run again, because the deps will match and the memoized result will be reused. We want memoized functions to run twice, too, so account for this, user functions are double invoked during the *first* invocation of the component function, and are *not* double invoked during the second incovation: - First execution of component function: user functions are double invoked - Second execution of component function (in Strict Mode, during development): user functions are not double invoked. It's hard to explain verbally but much clearer when you run the test cases I've added.
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: e7c5af4...fe59dfe Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show |
sebmarkbage
approved these changes
Oct 28, 2022
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Nov 1, 2022
Summary: This sync includes the following changes: - **[ab075a232](facebook/react@ab075a232 )**: Do not unmount layout effects on initial Offscreen mount ([#25592](facebook/react#25592)) //<Samuel Susla>// - **[765805bf8](facebook/react@765805bf8 )**: Fix type check for null ([#25595](facebook/react#25595)) //<Sebastian Markbåge>// - **[2ac77aab9](facebook/react@2ac77aab9 )**: Clean up vestige of useOpaqueIdentifier ([#25587](facebook/react#25587)) //<Andrew Clark>// - **[bdd3d0807](facebook/react@bdd3d0807 )**: Extract logic for detecting bad fallback to helper //<Andrew Clark>// - **[952dfff3f](facebook/react@952dfff3f )**: Split suspended work loop logic into separate functions //<Andrew Clark>// - **[d2c0ab10d](facebook/react@d2c0ab10d )**: In work loop, add enum of reasons for suspending //<Andrew Clark>// - **[5450dd409](facebook/react@5450dd409 )**: Strict Mode: Reuse memoized result from first pass ([#25583](facebook/react#25583)) //<Andrew Clark>// - **[d2a0176a1](facebook/react@d2a0176a1 )**: Detect and warn if use(promise) is wrapped with try/catch block ([#25543](facebook/react#25543)) //<Andrew Clark>// - **[cf3932be5](facebook/react@cf3932be5 )**: Remove old react-fetch, react-fs and react-pg libraries ([#25577](facebook/react#25577)) //<Sebastian Markbåge>// - **[28a574ea8](facebook/react@28a574ea8 )**: Try assigning fetch to globalThis if global assignment fails ([#25571](facebook/react#25571)) //<Sebastian Markbåge>// - **[09def5990](facebook/react@09def5990 )**: [Float] handle noscript context for Resources ([#25559](facebook/react#25559)) //<Josh Story>// - **[17204056d](facebook/react@17204056d )**: [Float] fix coordination of resource identity and hydration ([#25569](facebook/react#25569)) //<Josh Story>// - **[d925a8d0b](facebook/react@d925a8d0b )**: Flight client error stack ([#25560](facebook/react#25560)) //<Josh Story>// - **[996b00b78](facebook/react@996b00b78 )**: [Tiny] Fixed incorrect import in `react-server-dom-webpack` ([#25554](facebook/react#25554)) //<Leo Lamprecht>// - **[e7c5af45c](facebook/react@e7c5af45c )**: Update cache() and use() to the canary aka next channel ([#25502](facebook/react#25502)) //<Sebastian Markbåge>// - **[fa77f52e7](facebook/react@fa77f52e7 )**: Unify promise switch statements //<Andrew Clark>// - **[7572e4931](facebook/react@7572e4931 )**: Track thenable state in work loop //<Andrew Clark>// - **[7fc3eefd8](facebook/react@7fc3eefd8 )**: Revert yieldy behavior for non-use Suspense (in Flight, too) //<Andrew Clark>// - **[61f9b5e97](facebook/react@61f9b5e97 )**: [Float] support <base> as Resource ([#25546](facebook/react#25546)) //<Josh Story>// - **[1d3fc9c9c](facebook/react@1d3fc9c9c )**: Bug fix when resolving cache ([#25545](facebook/react#25545)) //<Sebastian Markbåge>// - **[cce18e350](facebook/react@cce18e350 )**: [Flight] Use AsyncLocalStorage to extend the scope of the cache to micro tasks ([#25542](facebook/react#25542)) //<Sebastian Markbåge>// - **[caa84c8da](facebook/react@caa84c8da )**: Revert fetch instrumentation to only RSC ([#25540](facebook/react#25540)) //<Sebastian Markbåge>// - **[0c11baa6a](facebook/react@0c11baa6a )**: add warnings for non-resources rendered outside body or head ([#25532](facebook/react#25532)) //<Josh Story>// - **[9236abdb5](facebook/react@9236abdb5 )**: when float is enabled only push title and script as a single unit ([#25536](facebook/react#25536)) //<Josh Story>// - **[dd5c20825](facebook/react@dd5c20825 )**: Revert yieldy behavior for non-use Suspense ([#25537](facebook/react#25537)) //<Andrew Clark>// - **[934177598](facebook/react@934177598 )**: fix transposed escape functions ([#25534](facebook/react#25534)) //<Josh Story>// - **[d1ced9fd5](facebook/react@d1ced9fd5 )**: [Float] support all links as Resources ([#25515](facebook/react#25515)) //<Josh Story>// - **[973b90bdf](facebook/react@973b90bdf )**: [Float] support meta tags as Resources ([#25514](facebook/react#25514)) //<Josh Story>// - **[79c582981](facebook/react@79c582981 )**: Let ReactDOM initialize in RSC ([#25503](facebook/react#25503)) //<Sebastian Markbåge>// - **[1f7a2f577](facebook/react@1f7a2f577 )**: [Float] support title tags as Resources ([#25508](facebook/react#25508)) //<Josh Story>// - **[c63580787](facebook/react@c63580787 )**: Support `use` in `act` testing API ([#25523](facebook/react#25523)) //<Andrew Clark>// - **[65e32e58b](facebook/react@65e32e58b )**: Add fetch Instrumentation to Dedupe Fetches ([#25516](facebook/react#25516)) //<Sebastian Markbåge>// - **[9336e29d9](facebook/react@9336e29d9 )**: [useEvent] Lint for presence of useEvent functions in dependency lists ([#25512](facebook/react#25512)) //<lauren>// - **[3cc792bfb](facebook/react@3cc792bfb )**: [useEvent] Non-stable function identity ([#25473](facebook/react#25473)) //<lauren>// - **[987292815](facebook/react@987292815 )**: Remove feature flag enableStrictEffects ([#25387](facebook/react#25387)) //<Samuel Susla>// - **[8e2bde6f2](facebook/react@8e2bde6f2 )**: Add cache() API ([#25506](facebook/react#25506)) //<Sebastian Markbåge>// - **[9cdf8a99e](facebook/react@9cdf8a99e )**: [Codemod] Update copyright header to Meta ([#25315](facebook/react#25315)) //<Andrew Clark>// - **[e54015e26](facebook/react@e54015e26 )**: Refactor: fill in the flow missing type ([#25496](facebook/react#25496)) //<c0dedance>// - **[3b1fd5767](facebook/react@3b1fd5767 )**: refactor: Flow: typing of Scheduler ([#25485](facebook/react#25485)) //<bubucuo>// - **[14072ce64](facebook/react@14072ce64 )**: Add detach to Offscreen component ([#25265](facebook/react#25265)) //<Samuel Susla>// - **[3bb71dfd4](facebook/react@3bb71dfd4 )**: Rename react-server-dom-webpack entry points to /client and /server ([#25504](facebook/react#25504)) //<Sebastian Markbåge>// - **[71f2c8cf1](facebook/react@71f2c8cf1 )**: move resource acquisition to mutation phase ([#25500](facebook/react#25500)) //<Josh Story>// - **[500bea532](facebook/react@500bea532 )**: Add option to load Fizz runtime from external file ([#25499](facebook/react#25499)) //<Andrew Clark>// - **[4494f2a86](facebook/react@4494f2a86 )**: [Float] add support for scripts and other enhancements ([#25480](facebook/react#25480)) //<Josh Story>// - **[9ecf84ed7](facebook/react@9ecf84ed7 )**: Bugfix: Suspending in shell during discrete update ([#25495](facebook/react#25495)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions 54f297a...ab075a2 jest_e2e[run_all_tests] Reviewed By: kassens Differential Revision: D40897093 fbshipit-source-id: 6a040315834dea5c0ab994ea94d91f5605b9d6b0
GrinZero
added a commit
to GrinZero/react
that referenced
this pull request
Nov 7, 2022
* 'main' of ssh://github.com/GrinZero/react: (163 commits) Do not unmount layout effects if ancestor Offscreen is hidden (facebook#25628) Remove check in renderDidSuspendDelayIfPossible (facebook#25630) [ServerRenderer] Move fizz external runtime implementation to react-dom-bindings (facebook#25617) Unwrap sync resolved thenables without suspending (facebook#25615) refactor isHostResourceType to not receive the context from reconciler and not leak types (facebook#25610) Make host context use null as empty and only error in dev (facebook#25609) [Float] handle resource Resource creation inside svg context (facebook#25599) Allow uncached IO to stablize (facebook#25561) [ServerRenderer] Setup for adding data attributes streaming format (facebook#25567) Do not unmount layout effects on initial Offscreen mount (facebook#25592) Fix type check for null (facebook#25595) Clean up vestige of useOpaqueIdentifier (facebook#25587) Extract logic for detecting bad fallback to helper Split suspended work loop logic into separate functions In work loop, add enum of reasons for suspending Strict Mode: Reuse memoized result from first pass (facebook#25583) Detect and warn if use(promise) is wrapped with try/catch block (facebook#25543) Remove old react-fetch, react-fs and react-pg libraries (facebook#25577) Try assigning fetch to globalThis if global assignment fails (facebook#25571) [Float] handle noscript context for Resources (facebook#25559) ... # Conflicts: # scripts/rollup/build.js
rickhanlonii
pushed a commit
that referenced
this pull request
Dec 3, 2022
In Strict Mode, during development, user functions are double invoked to help detect side effects. Currently, the way we implement this is to completely discard the first pass and start over. Theoretically this should be fine because components are idempotent. However, it's a bit tricky to get right because our implementation (i.e. `renderWithHooks`) is not completely idempotent with respect to internal data structures, like the work-in-progress fiber. In the past we've had to be really careful to avoid subtle bugs — for example, during the initial mount, `setState` functions are bound to the particular hook instances that were created during that render. If we compute new hook instances, we must also compute new children, and they must correspond to each other. This commit addresses a similar issue that came up related to `use`: when something suspends, `use` reuses the promise that was passed during the first attempt. This is itself a form of memoization. We need to be able to memoize the reactive inputs to the `use` call using a hook (i.e. `useMemo`), which means, the reactive inputs to `use` must come from the same component invocation as the output. The solution I've chosen is, rather than double invoke the entire `renderWithHook` function, we should double invoke each individual user function. It's a bit confusing but here's how it works: We will invoke the entire component function twice. However, during the second invocation of the component, the hook state from the first invocation will be reused. That means things like `useMemo` functions won't run again, because the deps will match and the memoized result will be reused. We want memoized functions to run twice, too, so account for this, user functions are double invoked during the *first* invocation of the component function, and are *not* double invoked during the second incovation: - First execution of component function: user functions are double invoked - Second execution of component function (in Strict Mode, during development): user functions are not double invoked. It's hard to explain verbally but much clearer when you run the test cases I've added.
OlimpiaZurek
pushed a commit
to OlimpiaZurek/react-native
that referenced
this pull request
May 22, 2023
Summary: This sync includes the following changes: - **[ab075a232](facebook/react@ab075a232 )**: Do not unmount layout effects on initial Offscreen mount ([facebook#25592](facebook/react#25592)) //<Samuel Susla>// - **[765805bf8](facebook/react@765805bf8 )**: Fix type check for null ([facebook#25595](facebook/react#25595)) //<Sebastian Markbåge>// - **[2ac77aab9](facebook/react@2ac77aab9 )**: Clean up vestige of useOpaqueIdentifier ([facebook#25587](facebook/react#25587)) //<Andrew Clark>// - **[bdd3d0807](facebook/react@bdd3d0807 )**: Extract logic for detecting bad fallback to helper //<Andrew Clark>// - **[952dfff3f](facebook/react@952dfff3f )**: Split suspended work loop logic into separate functions //<Andrew Clark>// - **[d2c0ab10d](facebook/react@d2c0ab10d )**: In work loop, add enum of reasons for suspending //<Andrew Clark>// - **[5450dd409](facebook/react@5450dd409 )**: Strict Mode: Reuse memoized result from first pass ([facebook#25583](facebook/react#25583)) //<Andrew Clark>// - **[d2a0176a1](facebook/react@d2a0176a1 )**: Detect and warn if use(promise) is wrapped with try/catch block ([facebook#25543](facebook/react#25543)) //<Andrew Clark>// - **[cf3932be5](facebook/react@cf3932be5 )**: Remove old react-fetch, react-fs and react-pg libraries ([facebook#25577](facebook/react#25577)) //<Sebastian Markbåge>// - **[28a574ea8](facebook/react@28a574ea8 )**: Try assigning fetch to globalThis if global assignment fails ([facebook#25571](facebook/react#25571)) //<Sebastian Markbåge>// - **[09def5990](facebook/react@09def5990 )**: [Float] handle noscript context for Resources ([facebook#25559](facebook/react#25559)) //<Josh Story>// - **[17204056d](facebook/react@17204056d )**: [Float] fix coordination of resource identity and hydration ([facebook#25569](facebook/react#25569)) //<Josh Story>// - **[d925a8d0b](facebook/react@d925a8d0b )**: Flight client error stack ([facebook#25560](facebook/react#25560)) //<Josh Story>// - **[996b00b78](facebook/react@996b00b78 )**: [Tiny] Fixed incorrect import in `react-server-dom-webpack` ([facebook#25554](facebook/react#25554)) //<Leo Lamprecht>// - **[e7c5af45c](facebook/react@e7c5af45c )**: Update cache() and use() to the canary aka next channel ([facebook#25502](facebook/react#25502)) //<Sebastian Markbåge>// - **[fa77f52e7](facebook/react@fa77f52e7 )**: Unify promise switch statements //<Andrew Clark>// - **[7572e4931](facebook/react@7572e4931 )**: Track thenable state in work loop //<Andrew Clark>// - **[7fc3eefd8](facebook/react@7fc3eefd8 )**: Revert yieldy behavior for non-use Suspense (in Flight, too) //<Andrew Clark>// - **[61f9b5e97](facebook/react@61f9b5e97 )**: [Float] support <base> as Resource ([facebook#25546](facebook/react#25546)) //<Josh Story>// - **[1d3fc9c9c](facebook/react@1d3fc9c9c )**: Bug fix when resolving cache ([facebook#25545](facebook/react#25545)) //<Sebastian Markbåge>// - **[cce18e350](facebook/react@cce18e350 )**: [Flight] Use AsyncLocalStorage to extend the scope of the cache to micro tasks ([facebook#25542](facebook/react#25542)) //<Sebastian Markbåge>// - **[caa84c8da](facebook/react@caa84c8da )**: Revert fetch instrumentation to only RSC ([facebook#25540](facebook/react#25540)) //<Sebastian Markbåge>// - **[0c11baa6a](facebook/react@0c11baa6a )**: add warnings for non-resources rendered outside body or head ([facebook#25532](facebook/react#25532)) //<Josh Story>// - **[9236abdb5](facebook/react@9236abdb5 )**: when float is enabled only push title and script as a single unit ([facebook#25536](facebook/react#25536)) //<Josh Story>// - **[dd5c20825](facebook/react@dd5c20825 )**: Revert yieldy behavior for non-use Suspense ([facebook#25537](facebook/react#25537)) //<Andrew Clark>// - **[934177598](facebook/react@934177598 )**: fix transposed escape functions ([facebook#25534](facebook/react#25534)) //<Josh Story>// - **[d1ced9fd5](facebook/react@d1ced9fd5 )**: [Float] support all links as Resources ([facebook#25515](facebook/react#25515)) //<Josh Story>// - **[973b90bdf](facebook/react@973b90bdf )**: [Float] support meta tags as Resources ([facebook#25514](facebook/react#25514)) //<Josh Story>// - **[79c582981](facebook/react@79c582981 )**: Let ReactDOM initialize in RSC ([facebook#25503](facebook/react#25503)) //<Sebastian Markbåge>// - **[1f7a2f577](facebook/react@1f7a2f577 )**: [Float] support title tags as Resources ([facebook#25508](facebook/react#25508)) //<Josh Story>// - **[c63580787](facebook/react@c63580787 )**: Support `use` in `act` testing API ([facebook#25523](facebook/react#25523)) //<Andrew Clark>// - **[65e32e58b](facebook/react@65e32e58b )**: Add fetch Instrumentation to Dedupe Fetches ([facebook#25516](facebook/react#25516)) //<Sebastian Markbåge>// - **[9336e29d9](facebook/react@9336e29d9 )**: [useEvent] Lint for presence of useEvent functions in dependency lists ([facebook#25512](facebook/react#25512)) //<lauren>// - **[3cc792bfb](facebook/react@3cc792bfb )**: [useEvent] Non-stable function identity ([facebook#25473](facebook/react#25473)) //<lauren>// - **[987292815](facebook/react@987292815 )**: Remove feature flag enableStrictEffects ([facebook#25387](facebook/react#25387)) //<Samuel Susla>// - **[8e2bde6f2](facebook/react@8e2bde6f2 )**: Add cache() API ([facebook#25506](facebook/react#25506)) //<Sebastian Markbåge>// - **[9cdf8a99e](facebook/react@9cdf8a99e )**: [Codemod] Update copyright header to Meta ([facebook#25315](facebook/react#25315)) //<Andrew Clark>// - **[e54015e26](facebook/react@e54015e26 )**: Refactor: fill in the flow missing type ([facebook#25496](facebook/react#25496)) //<c0dedance>// - **[3b1fd5767](facebook/react@3b1fd5767 )**: refactor: Flow: typing of Scheduler ([facebook#25485](facebook/react#25485)) //<bubucuo>// - **[14072ce64](facebook/react@14072ce64 )**: Add detach to Offscreen component ([facebook#25265](facebook/react#25265)) //<Samuel Susla>// - **[3bb71dfd4](facebook/react@3bb71dfd4 )**: Rename react-server-dom-webpack entry points to /client and /server ([facebook#25504](facebook/react#25504)) //<Sebastian Markbåge>// - **[71f2c8cf1](facebook/react@71f2c8cf1 )**: move resource acquisition to mutation phase ([facebook#25500](facebook/react#25500)) //<Josh Story>// - **[500bea532](facebook/react@500bea532 )**: Add option to load Fizz runtime from external file ([facebook#25499](facebook/react#25499)) //<Andrew Clark>// - **[4494f2a86](facebook/react@4494f2a86 )**: [Float] add support for scripts and other enhancements ([facebook#25480](facebook/react#25480)) //<Josh Story>// - **[9ecf84ed7](facebook/react@9ecf84ed7 )**: Bugfix: Suspending in shell during discrete update ([facebook#25495](facebook/react#25495)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions 54f297a...ab075a2 jest_e2e[run_all_tests] Reviewed By: kassens Differential Revision: D40897093 fbshipit-source-id: 6a040315834dea5c0ab994ea94d91f5605b9d6b0
|
Before this change, state initializer functions passed to I assume it's intentional that they're not being manually called a second time (like |
gsathya
added a commit
to gsathya/react
that referenced
this pull request
Feb 5, 2024
This fixes a regression facebook#25583 where we stopped patching before calling useMemo function.
gsathya
added a commit
to gsathya/react
that referenced
this pull request
Feb 6, 2024
This fixes a regression facebook#25583 where we stopped patching before calling useMemo function.
gsathya
added a commit
that referenced
this pull request
Feb 6, 2024
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
…ok#28249) This fixes a regression facebook#25583 where we stopped patching before calling useMemo function. Fixes facebook#27989
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.
In Strict Mode, during development, user functions are double invoked to help detect side effects. Currently, the way we implement this is to completely discard the first pass and start over. Theoretically this should be fine because components are idempotent. However, it's a bit tricky to get right because our implementation (i.e.
renderWithHooks) is not completely idempotent with respect to internal data structures, like the work-in-progress fiber. In the past we've had to be really careful to avoid subtle bugs — for example, during the initial mount,setStatefunctions are bound to the particular hook instances that were created during that render. If we compute new hook instances, we must also compute new children, and they must correspond to each other.This commit addresses a similar issue that came up related to
use: when something suspends,usereuses the promise that was passed during the first attempt. This is itself a form of memoization. We need to be able to memoize the reactive inputs to theusecall using a hook (i.e.useMemo), which means, the reactive inputs tousemust come from the same component invocation as the output.The solution I've chosen is, rather than double invoke the entire
renderWithHookfunction, we should double invoke each individual user function. It's a bit confusing but here's how it works:We will invoke the entire component function twice. However, during the second invocation of the component, the hook state from the first invocation will be reused. That means things like
useMemofunctions won't run again, because the deps will match and the memoized result will be reused.We want memoized functions to run twice, too, so account for this, user functions are double invoked during the first invocation of the component function, and are not double invoked during the second incovation:
It's hard to explain verbally but much clearer when you run the test cases I've added.