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

fix[devtools/useMemoCache]: implement a working copy of useMemoCache #27659

Merged
merged 1 commit into from
Nov 14, 2023

Conversation

hoxyq
Copy link
Contributor

@hoxyq hoxyq commented Nov 6, 2023

In #27472 I've removed broken useMemoCache implementation and replaced it with a stub. It actually produces errors when trying to inspect components, which are compiled with Forget.

The main difference from the implementation in #26696 is that we are using corresponding Fiber here, which has patched updateQueue with memoCache. Previously we would check it on a hook object, which doesn't have updateQueue.

Tested on pages, which are using Forget and by inspecting elements, which are transpiled with Forget.

@hoxyq hoxyq requested review from gsathya and poteto November 6, 2023 22:27
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Nov 6, 2023
@react-sizebot
Copy link

react-sizebot commented Nov 6, 2023

Comparing: 0c63487...105169b

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.min.js = 175.09 kB 175.09 kB = 54.48 kB 54.48 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 177.21 kB 177.21 kB = 55.16 kB 55.16 kB
facebook-www/ReactDOM-prod.classic.js = 567.70 kB 567.70 kB = 99.96 kB 99.96 kB
facebook-www/ReactDOM-prod.modern.js = 551.56 kB 551.56 kB = 97.06 kB 97.06 kB
oss-experimental/react-debug-tools/cjs/react-debug-tools.production.min.js +3.69% 6.83 kB 7.09 kB +4.60% 2.54 kB 2.66 kB
oss-stable-semver/react-debug-tools/cjs/react-debug-tools.production.min.js +3.69% 6.83 kB 7.09 kB +4.60% 2.54 kB 2.66 kB
oss-stable/react-debug-tools/cjs/react-debug-tools.production.min.js +3.69% 6.83 kB 7.09 kB +4.60% 2.54 kB 2.66 kB
oss-experimental/react-debug-tools/cjs/react-debug-tools.development.js +3.33% 22.69 kB 23.44 kB +3.80% 6.18 kB 6.41 kB
oss-stable-semver/react-debug-tools/cjs/react-debug-tools.development.js +3.33% 22.69 kB 23.44 kB +3.80% 6.18 kB 6.41 kB
oss-stable/react-debug-tools/cjs/react-debug-tools.development.js +3.33% 22.69 kB 23.44 kB +3.80% 6.18 kB 6.41 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/react-debug-tools/cjs/react-debug-tools.production.min.js +3.69% 6.83 kB 7.09 kB +4.60% 2.54 kB 2.66 kB
oss-stable-semver/react-debug-tools/cjs/react-debug-tools.production.min.js +3.69% 6.83 kB 7.09 kB +4.60% 2.54 kB 2.66 kB
oss-stable/react-debug-tools/cjs/react-debug-tools.production.min.js +3.69% 6.83 kB 7.09 kB +4.60% 2.54 kB 2.66 kB
oss-experimental/react-debug-tools/cjs/react-debug-tools.development.js +3.33% 22.69 kB 23.44 kB +3.80% 6.18 kB 6.41 kB
oss-stable-semver/react-debug-tools/cjs/react-debug-tools.development.js +3.33% 22.69 kB 23.44 kB +3.80% 6.18 kB 6.41 kB
oss-stable/react-debug-tools/cjs/react-debug-tools.development.js +3.33% 22.69 kB 23.44 kB +3.80% 6.18 kB 6.41 kB

Generated by 🚫 dangerJS against 105169b


const memoCache = fiber.updateQueue?.memoCache;
if (memoCache == null) {
return [];
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this case need to write the cache?

Where is this implementation coming from?

Copy link
Member

Choose a reason for hiding this comment

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

Compared to

function useMemoCache(size: number): Array<any> {
it generally seems to be doing a lot less. Could this just call the "real implementation", esp if it doesn't need to do any extra record keeping?

Copy link
Contributor

Choose a reason for hiding this comment

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

i think the main difference is that it's using syntax sugar which the real version has to avoid because we prefer to avoid the bloated expansion of optional chains and such. this looks right at least, though +1 to just reusing the same implementation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should not do the same thing as the original implementation. Other hook implementations in this file actually don't mutate anything in Fiber or Hook object.

This can't be applied for useMemoCache and the main reason is its implementation: although it is a hook, it is not implemented in the same way as others. It doesn't update memoizedState on the Fiber, so we could see it in the chain with other hooks.

When inspectHooksOfFiber is called, it gets a Fiber as an argument, which is already "rendered", so we could traverse through Fiber.memoizedState and return values from there. For some reason, this doesn't work with useMemoCache, I've tried returning just memoCache.data[memoCache.index], but this implementation produces errors in the render function of the inspected component. I am guessing that it is somewhat related to useMemoCache's implementation.

@hoxyq hoxyq force-pushed the devtools/use-memo-cache branch 2 times, most recently from 05e7994 to fba271f Compare November 7, 2023 12:18
@hoxyq hoxyq force-pushed the devtools/use-memo-cache branch from fba271f to 105169b Compare November 7, 2023 13:20
Copy link
Contributor

@josephsavona josephsavona left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks for explaining why this can't reuse the original implementation (and for implementing this, of course!)

@hoxyq hoxyq merged commit aec521a into facebook:main Nov 14, 2023
2 checks passed
@hoxyq hoxyq deleted the devtools/use-memo-cache branch November 14, 2023 18:23
kodiakhq bot pushed a commit to vercel/next.js that referenced this pull request Nov 16, 2023
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…acebook#27659)

In facebook#27472 I've removed broken
`useMemoCache` implementation and replaced it with a stub. It actually
produces errors when trying to inspect components, which are compiled
with Forget.

The main difference from the implementation in
facebook#26696 is that we are using
corresponding `Fiber` here, which has patched `updateQueue` with
`memoCache`. Previously we would check it on a hook object, which
doesn't have `updateQueue`.

Tested on pages, which are using Forget and by inspecting elements,
which are transpiled with Forget.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
…27659)

In #27472 I've removed broken
`useMemoCache` implementation and replaced it with a stub. It actually
produces errors when trying to inspect components, which are compiled
with Forget.

The main difference from the implementation in
#26696 is that we are using
corresponding `Fiber` here, which has patched `updateQueue` with
`memoCache`. Previously we would check it on a hook object, which
doesn't have `updateQueue`.

Tested on pages, which are using Forget and by inspecting elements,
which are transpiled with Forget.

DiffTrain build for commit aec521a.
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