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[ReactDebugHooks/find-primitive-index]: remove some assumptions #29652

Conversation

hoxyq
Copy link
Contributor

@hoxyq hoxyq commented May 29, 2024

Partially reverts #28593.

While rolling out RDT 5.2.0, I've observed some issues on React Native side: hooks inspection for some complex hook trees, like in AnimatedView, were broken. After some debugging, I've noticed a difference between what is in frame's source.

The difference is in the top-most frame, where with V8 it will correctly pick up the Type as Proxy in hookStack, but for Hermes it will be Object. This means that for React Native this top most frame is skipped, since sources are identical.

Here I am reverting back to the previous logic, where we check each frame if its a part of the wrapper, but also updated isReactWrapper function to have an explicit case for useFormStatus support.

@hoxyq hoxyq requested a review from eps1lon May 29, 2024 18:06
Copy link

vercel bot commented May 29, 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 May 30, 2024 2:02pm

@react-sizebot
Copy link

react-sizebot commented May 29, 2024

Comparing: afb2c39...6a0b69c

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.66 kB 6.66 kB +0.17% 1.82 kB 1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 496.38 kB 496.38 kB = 88.84 kB 88.84 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.67 kB 6.67 kB +0.11% 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 501.20 kB 501.20 kB = 89.53 kB 89.53 kB
facebook-www/ReactDOM-prod.classic.js = 593.81 kB 593.81 kB = 104.45 kB 104.45 kB
facebook-www/ReactDOM-prod.modern.js = 570.20 kB 570.20 kB = 100.85 kB 100.85 kB
test_utils/ReactAllWarnings.js Deleted 63.65 kB 0.00 kB Deleted 15.90 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable-semver/react-debug-tools/cjs/react-debug-tools.production.js +1.27% 27.88 kB 28.24 kB +0.61% 5.55 kB 5.58 kB
oss-stable/react-debug-tools/cjs/react-debug-tools.production.js +1.27% 27.88 kB 28.24 kB +0.61% 5.55 kB 5.58 kB
oss-experimental/react-debug-tools/cjs/react-debug-tools.production.js +1.21% 27.90 kB 28.24 kB +0.58% 5.55 kB 5.58 kB
oss-experimental/react-debug-tools/cjs/react-debug-tools.development.js +0.67% 36.54 kB 36.78 kB +0.50% 8.72 kB 8.77 kB
oss-stable-semver/react-debug-tools/cjs/react-debug-tools.development.js +0.67% 36.54 kB 36.78 kB +0.50% 8.72 kB 8.77 kB
oss-stable/react-debug-tools/cjs/react-debug-tools.development.js +0.67% 36.54 kB 36.78 kB +0.50% 8.72 kB 8.77 kB
test_utils/ReactAllWarnings.js Deleted 63.65 kB 0.00 kB Deleted 15.90 kB 0.00 kB

Generated by 🚫 dangerJS against 6a0b69c

// assume that the next frame after that is the actual public API call.
// This prohibits nesting dispatcher calls in hooks.
// If the next two frames are functions called `useX` then we assume that they're part of the
// wrappers that the React packager or other packages adds around the dispatcher.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// wrappers that the React packager or other packages adds around the dispatcher.
// wrappers that the React package or other packages adds around the dispatcher.

@hoxyq hoxyq force-pushed the react-devtools/fix-find-primitive-index-for-react-native branch from 14bdcd3 to 6a0b69c Compare May 30, 2024 13:56
@hoxyq hoxyq merged commit fb61a1b into facebook:main May 30, 2024
40 checks passed
@hoxyq hoxyq deleted the react-devtools/fix-find-primitive-index-for-react-native branch May 30, 2024 15:12
github-actions bot pushed a commit that referenced this pull request May 30, 2024
…29652)

Partially reverts #28593.

While rolling out RDT 5.2.0, I've observed some issues on React Native
side: hooks inspection for some complex hook trees, like in
AnimatedView, were broken. After some debugging, I've noticed a
difference between what is in frame's source.

The difference is in the top-most frame, where with V8 it will correctly
pick up the `Type` as `Proxy` in `hookStack`, but for Hermes it will be
`Object`. This means that for React Native this top most frame is
skipped, since sources are identical.

Here I am reverting back to the previous logic, where we check each
frame if its a part of the wrapper, but also updated `isReactWrapper`
function to have an explicit case for `useFormStatus` support.

DiffTrain build for commit fb61a1b.
github-actions bot pushed a commit that referenced this pull request May 30, 2024
…29652)

Partially reverts #28593.

While rolling out RDT 5.2.0, I've observed some issues on React Native
side: hooks inspection for some complex hook trees, like in
AnimatedView, were broken. After some debugging, I've noticed a
difference between what is in frame's source.

The difference is in the top-most frame, where with V8 it will correctly
pick up the `Type` as `Proxy` in `hookStack`, but for Hermes it will be
`Object`. This means that for React Native this top most frame is
skipped, since sources are identical.

Here I am reverting back to the previous logic, where we check each
frame if its a part of the wrapper, but also updated `isReactWrapper`
function to have an explicit case for `useFormStatus` support.

DiffTrain build for [fb61a1b](fb61a1b)
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