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

[fresh] Reset useMemoCache in response to a FastRefresh run #30677

Closed
wants to merge 4 commits into from

Conversation

poteto
Copy link
Member

@poteto poteto commented Aug 13, 2024

Stack from ghstack (oldest at bottom):

This PR introduces a new (dev only) debug property on fibers to indicate
that it is being rerendered as a result of a FastRefresh run. This
allows React to clear the cache even where the size (incidentally)
remains constant between renders: for example, if edited code happens
to keep the cache size the same but the code is meaningfully different
such that it is no longer correct to reuse the cache.

[ghstack-poisoned]
Copy link

vercel bot commented Aug 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 14, 2024 6:54pm

poteto added a commit that referenced this pull request Aug 13, 2024
This PR introduces a new debug property on fibers to indicate that it is
being rerendered as a result of a FastRefresh run. This allows React to
clear the cache even where the size (incidentally) remains constant
between renders: for example, if edited code _happens_ to keep the cache
size the same but the code is meaningfully different such that it is no
longer correct to reuse the cache.

ghstack-source-id: 4984a3834adb18af06ffdbd5700406e1ba29a60d
Pull Request resolved: #30677
@facebook-github-bot facebook-github-bot added the React Core Team Opened by a member of the React Core Team label Aug 13, 2024
@react-sizebot
Copy link

react-sizebot commented Aug 13, 2024

Comparing: dd8e0ba...d2d5f30

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.68 kB 6.68 kB +0.05% 1.82 kB 1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js +0.02% 500.37 kB 500.48 kB +0.04% 89.80 kB 89.84 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.69 kB 6.69 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js +0.02% 507.50 kB 507.61 kB +0.04% 90.96 kB 91.00 kB
facebook-www/ReactDOM-prod.classic.js +0.02% 595.24 kB 595.35 kB +0.03% 105.55 kB 105.59 kB
facebook-www/ReactDOM-prod.modern.js +0.02% 571.54 kB 571.65 kB +0.04% 101.75 kB 101.78 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
test_utils/ReactAllWarnings.js = 64.07 kB 63.92 kB = 15.86 kB 15.81 kB

Generated by 🚫 dangerJS against d0b36cb

poteto added a commit that referenced this pull request Aug 13, 2024
This PR introduces a new (dev only) debug property on fibers to indicate
that it is being rerendered as a result of a FastRefresh run. This
allows React to clear the cache even where the size (incidentally)
remains constant between renders: for example, if edited code _happens_
to keep the cache size the same but the code is meaningfully different
such that it is no longer correct to reuse the cache.

ghstack-source-id: 4984a3834adb18af06ffdbd5700406e1ba29a60d
Pull Request resolved: #30677
@poteto poteto changed the title [fresh] Reset cache in response to a FastRefresh run [fresh] Reset useMemoCache in response to a FastRefresh run Aug 13, 2024
poteto added a commit that referenced this pull request Aug 13, 2024
This PR introduces a new (dev only) debug property on fibers to indicate
that it is being rerendered as a result of a FastRefresh run. This
allows React to clear the cache even where the size (incidentally)
remains constant between renders: for example, if edited code _happens_
to keep the cache size the same but the code is meaningfully different
such that it is no longer correct to reuse the cache.

ghstack-source-id: 4984a3834adb18af06ffdbd5700406e1ba29a60d
Pull Request resolved: #30677
[ghstack-poisoned]
poteto added a commit that referenced this pull request Aug 13, 2024
This PR introduces a new (dev only) debug property on fibers to indicate
that it is being rerendered as a result of a FastRefresh run. This
allows React to clear the cache even where the size (incidentally)
remains constant between renders: for example, if edited code _happens_
to keep the cache size the same but the code is meaningfully different
such that it is no longer correct to reuse the cache.

ghstack-source-id: 9bf49f9ea20ca785c93d32c2f1f720d225d7d764
Pull Request resolved: #30677
@poteto poteto requested a review from gaearon August 14, 2024 14:45
@gaearon
Copy link
Collaborator

gaearon commented Aug 14, 2024

Is it possible to use the module-level ignorePreviousDependencies for this, which should be getting set in the right places?

@gaearon
Copy link
Collaborator

gaearon commented Aug 14, 2024

Also noticing the test passes even if I comment out currentlyRenderingFiber._debugNeedsMemoCacheReset === true

[ghstack-poisoned]
poteto added a commit that referenced this pull request Aug 14, 2024
This PR introduces a new (dev only) debug property on fibers to indicate
that it is being rerendered as a result of a FastRefresh run. This
allows React to clear the cache even where the size (incidentally)
remains constant between renders: for example, if edited code _happens_
to keep the cache size the same but the code is meaningfully different
such that it is no longer correct to reuse the cache.

ghstack-source-id: d4b47584ad600e908ef51d271a96e89617bdb4e5
Pull Request resolved: #30677
[ghstack-poisoned]
poteto added a commit that referenced this pull request Aug 14, 2024
This PR introduces a new (dev only) debug property on fibers to indicate
that it is being rerendered as a result of a FastRefresh run. This
allows React to clear the cache even where the size (incidentally)
remains constant between renders: for example, if edited code _happens_
to keep the cache size the same but the code is meaningfully different
such that it is no longer correct to reuse the cache.

ghstack-source-id: 2e14ae4bf8aaf9070848138901585d47c2a98e21
Pull Request resolved: #30677
@gaearon
Copy link
Collaborator

gaearon commented Aug 14, 2024

I think #30700 makes sense to me, any reason why we don't do that instead?

@poteto
Copy link
Member Author

poteto commented Aug 14, 2024

Closing in favor of #30700

@poteto poteto closed this Aug 14, 2024
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 <[email protected]>
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 <[email protected]>

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 <[email protected]>

DiffTrain build for commit 7e8a06c.
@poteto poteto deleted the gh/poteto/3/head branch October 7, 2024 22:08
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