[fresh] Reset useMemoCache when size changes #30663
Closed
+41
−13
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
poteto
added a commit
that referenced
this pull request
Aug 12, 2024
During development when adding or removing code, it is common for the cache size to vary between refreshes. Rather than warn like before, let's simply and safely reset the cache completely whenever its size changes between renders. In practice, in non-development environments full invalidation should never happen since the only consumer of uMC is compiled code – which always emits a static call to uMC with a fixed size. ghstack-source-id: 2f17b959d126672751f1c9b2fe0026800d8ba174 Pull Request resolved: #30663
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: dd8e0ba...e007738 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
|
poteto
commented
Aug 12, 2024
Comment on lines
1237
to
1240
| data = memoCache.data[memoCache.index] = new Array(size); | ||
| for (let i = 0; i < size; i++) { | ||
| data[i] = REACT_MEMO_CACHE_SENTINEL; | ||
| } |
There was a problem hiding this comment.
An alternative approach might be to append a debug property on the fiber during fast refresh to reset the cache only when Fresh actually kicks in. That has the advantage of resetting the cache during a refresh even when cache size is the same (ie new code happened to result in the same number of cache slots)
There was a problem hiding this comment.
This seems to be the better approach to me.
- React shouldn't be throwing prod-only errors.
- Feels very likely there are types of changes where the memo size doesn't change (e.g. some literal such as a style is updated in the component)
There was a problem hiding this comment.
What does useMemo do if its implementation changes? Does it just keep its cache entry? Seems like it would need to reset for the same reason.
There was a problem hiding this comment.
useMemo always drops the cache on hot reload. Seems like we should do the same for uMC? We assume memoization is not essential for the logic and can be reconstructed. This also tests that the component logic is resilient to it somewhat.
poteto
added a commit
that referenced
this pull request
Aug 12, 2024
During development when adding or removing code, it is common for the cache size to vary between refreshes. Rather than warn like before, let's simply and safely reset the cache completely whenever its size changes between renders. In practice, in non-development environments full invalidation should never happen since the only consumer of uMC is compiled code – which always emits a static call to uMC with a fixed size. ghstack-source-id: 48c5b9c848a0b9ed01576ad357e067b72e8f887f Pull Request resolved: #30663
poteto
added a commit
that referenced
this pull request
Aug 12, 2024
During development when adding or removing code, it is common for the cache size to vary between refreshes. Rather than warn like before, let's simply and safely reset the cache completely whenever its size changes between renders. In practice, in non-development environments full invalidation should never happen since the only consumer of uMC is compiled code – which always emits a static call to uMC with a fixed size. ghstack-source-id: bffbd19f2b0fad935d2c51e6c3f129bd5900b9be Pull Request resolved: #30663
poteto
added a commit
that referenced
this pull request
Aug 12, 2024
During development when adding or removing code, it is common for the cache size to vary between refreshes. Rather than warn like before, let's simply and safely reset the cache completely whenever its size changes between renders. In practice, in non-development environments full invalidation should never happen since the only consumer of uMC is compiled code – which always emits a static call to uMC with a fixed size. ghstack-source-id: 48c5b9c848a0b9ed01576ad357e067b72e8f887f Pull Request resolved: #30663
poteto
added a commit
that referenced
this pull request
Aug 12, 2024
During development when adding or removing code, it is common for the cache size to vary between refreshes. Rather than warn like before, let's simply and safely reset the cache completely whenever its size changes between renders. In practice, in non-development environments full invalidation should never happen since the only consumer of uMC is compiled code – which always emits a static call to uMC with a fixed size. ghstack-source-id: 3fa62480e8bc89fc3c8593937be75290864d88d3 Pull Request resolved: #30663
There was a problem hiding this comment.
Throwing in prod for something that works in dev can lead to problems. I think we might need to either a) specifically limit this to fast refresh in dev (so that it errors otherwise) or b) just reset even in prod.
But I can see the argument that since you are never supposed to use this API directly, that it's maybe fine for it to have different behavior in dev/prod.
|
@josephsavona Yeah I was kinda torn about it but I was thinking that there should be no case ever where the cache size would change in between renders in prod, so it's better to error early. Alternatively (as you also pointed out in (b)) we can always reset the cache if the size changes in any environment, which would then eliminate the dev/prod divergence |
|
Per offline discussion let's go with resetting whenever the size changes. Seems like we're agreed that the ideal way to do this is for React to get a signal that a particular component fast refreshed and to reset the cache only then. But let's save that for a follow-up. |
poteto
added a commit
that referenced
this pull request
Aug 13, 2024
During development when adding or removing code, it is common for the cache size to vary between refreshes. Rather than warn like before, let's simply and safely reset the cache completely whenever its size changes between renders. In practice, in non-development environments full invalidation should never happen since the only consumer of uMC is compiled code – which always emits a static call to uMC with a fixed size. However, for consistency we continue to reset the cache as well. ghstack-source-id: b295a5d79692484f8685974dea2d6ce3954ef855 Pull Request resolved: #30663
poteto
added a commit
that referenced
this pull request
Aug 13, 2024
During development when adding or removing code, it is common for the cache size to vary between refreshes. Rather than warn like before, let's simply and safely reset the cache completely whenever its size changes between renders. In practice, in non-development environments full invalidation should never happen since the only consumer of uMC is compiled code – which always emits a static call to uMC with a fixed size. However, for consistency we continue to reset the cache as well. ghstack-source-id: b295a5d79692484f8685974dea2d6ce3954ef855 Pull Request resolved: #30663
poteto
added a commit
that referenced
this pull request
Aug 13, 2024
During development when adding or removing code, it is common for the cache size to vary between refreshes. Rather than warn like before, let's simply and safely reset the cache completely whenever its size changes between renders. In practice, in non-development environments full invalidation should never happen since the only consumer of uMC is compiled code – which always emits a static call to uMC with a fixed size. However, for consistency we continue to reset the cache as well. ghstack-source-id: 7f343a7e7d4c0575146c9c9148ab2d34137d578d Pull Request resolved: #30663
poteto
added a commit
that referenced
this pull request
Aug 13, 2024
During development when adding or removing code, it is common for the cache size to vary between refreshes. Rather than warn like before, let's simply and safely reset the cache completely whenever its size changes between renders. In practice, in non-development environments full invalidation should never happen since the only consumer of uMC is compiled code – which always emits a static call to uMC with a fixed size. However, for consistency we continue to reset the cache as well. ghstack-source-id: 440a8997bf61448ac01ce78d5f58e1e1b880fa83 Pull Request resolved: #30663
|
@josephsavona updated |
|
Why is it not sufficient to always throw away the cache on hot reload? Unless I’m missing something, that’s what we do for useMemo. |
|
Closing in favor of #30700 |
gaearon
added a commit
that referenced
this pull request
Aug 15, 2024
Stacked on #30662. Alternative to #30663 and #30677. During a Fast Refresh, we always want to evict the memo cache, same as we do with normal `useMemo`. The mechanism used by `useMemo` and other Hooks is this module-level variable: https://github.com/facebook/react/blob/fca5d655d78917400a2722287351c20938166669/packages/react-reconciler/src/ReactFiberHooks.js#L304-L307 which has DEV-only behavior as if the dependencies are always different: https://github.com/facebook/react/blob/fca5d655d78917400a2722287351c20938166669/packages/react-reconciler/src/ReactFiberHooks.js#L451-L460 The `useMemoCache` Hook doesn't use a dependency array but conceptually I think we want the same behavior. ## Test Plan The test passes. --------- Co-authored-by: Lauren Tan <poteto@users.noreply.github.com>
github-actions bot
pushed a commit
that referenced
this pull request
Aug 15, 2024
Stacked on #30662. Alternative to #30663 and #30677. During a Fast Refresh, we always want to evict the memo cache, same as we do with normal `useMemo`. The mechanism used by `useMemo` and other Hooks is this module-level variable: https://github.com/facebook/react/blob/fca5d655d78917400a2722287351c20938166669/packages/react-reconciler/src/ReactFiberHooks.js#L304-L307 which has DEV-only behavior as if the dependencies are always different: https://github.com/facebook/react/blob/fca5d655d78917400a2722287351c20938166669/packages/react-reconciler/src/ReactFiberHooks.js#L451-L460 The `useMemoCache` Hook doesn't use a dependency array but conceptually I think we want the same behavior. ## Test Plan The test passes. --------- Co-authored-by: Lauren Tan <poteto@users.noreply.github.com> DiffTrain build for [7e8a06c](7e8a06c)
github-actions bot
pushed a commit
that referenced
this pull request
Aug 15, 2024
Stacked on #30662. Alternative to #30663 and #30677. During a Fast Refresh, we always want to evict the memo cache, same as we do with normal `useMemo`. The mechanism used by `useMemo` and other Hooks is this module-level variable: https://github.com/facebook/react/blob/fca5d655d78917400a2722287351c20938166669/packages/react-reconciler/src/ReactFiberHooks.js#L304-L307 which has DEV-only behavior as if the dependencies are always different: https://github.com/facebook/react/blob/fca5d655d78917400a2722287351c20938166669/packages/react-reconciler/src/ReactFiberHooks.js#L451-L460 The `useMemoCache` Hook doesn't use a dependency array but conceptually I think we want the same behavior. ## Test Plan The test passes. --------- Co-authored-by: Lauren Tan <poteto@users.noreply.github.com> DiffTrain build for commit 7e8a06c.
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.
Stack from ghstack (oldest at bottom):
During development when adding or removing code, it is common for the
cache size to vary between refreshes. Rather than warn like before,
let's simply and safely reset the cache completely whenever its size
changes between renders. In practice, in non-development environments
full invalidation should never happen since the only consumer of uMC is
compiled code – which always emits a static call to uMC with a fixed
size. However, for consistency we continue to reset the cache as well.