Skip to content

[DevTools Bug]: Hook parsing fails if a hook uses useSyncExternalStore #27889

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

Closed
jamesbvaughan opened this issue Jan 7, 2024 · 9 comments · Fixed by #28399
Closed

[DevTools Bug]: Hook parsing fails if a hook uses useSyncExternalStore #27889

jamesbvaughan opened this issue Jan 7, 2024 · 9 comments · Fixed by #28399
Assignees
Labels
Component: Developer Tools Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Type: Bug

Comments

@jamesbvaughan
Copy link
Contributor

jamesbvaughan commented Jan 7, 2024

Website or app

https://react-devtools-issue-demo.vercel.app/

Repro steps

  1. Open up this page: https://react-devtools-issue-demo.vercel.app/ (source: https://github.com/jamesbvaughan/react-devtools-issue-demo/blob/main/src/App.tsx)
  2. Open up the React dev tools
  3. Navigate to the Components tab
  4. Observe in the right sidebar that hook name parsing fails for one of the two components.

Hook name parsing is really helpful when debugging/optimizing large components with lots of hooks, and this issue makes that feature unusable on components whose hooks make use of useSyncExternalStore.

This is particularly painful for my current work, where nearly all components make use of Zustand stores, which use useSyncExternalStore internally.


This is the same symptom that folks are hitting in #24980, but I'm not sure whether the cause is the same for everyone there.

How often does this bug happen?

Every time

DevTools package (automated)

No response

DevTools version (automated)

No response

Error message (automated)

No response

Error call stack (automated)

No response

Error component stack (automated)

No response

GitHub query string (automated)

No response

@jamesbvaughan
Copy link
Contributor Author

I've made a bit more progress investigating this.

I believe the cause might be the two calls to nextHook in useSyncExternalStore:

function useSyncExternalStore<T>(
subscribe: (() => void) => () => void,
getSnapshot: () => T,
getServerSnapshot?: () => T,
): T {
// useSyncExternalStore() composes multiple hooks internally.
// Advance the current hook index the same number of times
// so that subsequent hooks have the right memoized state.
nextHook(); // SyncExternalStore
nextHook(); // Effect

To try to verify that, I found that just one other hook in that file, useTransition, also makes two calls to nextHook:

function useTransition(): [
boolean,
(callback: () => void, options?: StartTransitionOptions) => void,
] {
// useTransition() composes multiple hooks internally.
// Advance the current hook index the same number of times
// so that subsequent hooks have the right memoized state.
nextHook(); // State
nextHook(); // Callback

I modified my test case to use useTransition rather than useSyncExternalStore and confirmed that it also causes hook parsing to fail.

@hoxyq hoxyq self-assigned this Jan 7, 2024
@ganialhamad
Copy link

hey @hoxyq did you work on this?

@hoxyq
Copy link
Contributor

hoxyq commented Jan 11, 2024

@jamesbvaughan, thanks for reporting this! Will take a look at it in a few days and reply back.

@Nathan-Roberts123
Copy link

function getPrimitiveStackCache(): Map<string, Array<any>> {
// This initializes a cache of all primitive hooks so that the top
// most stack frames added by calling the primitive hook can be removed.
if (primitiveStackCache === null) {
const cache = new Map<string, Array<any>>();
let readHookLog;
try {
// Use all hooks here to add them to the hook log.
Dispatcher.useContext(({_currentValue: null}: any));
Dispatcher.useState(null);
Dispatcher.useReducer((s: mixed, a: mixed) => s, null);
Dispatcher.useRef(null);
if (typeof Dispatcher.useCacheRefresh === 'function') {
// This type check is for Flow only.
Dispatcher.useCacheRefresh();
}
Dispatcher.useLayoutEffect(() => {});
Dispatcher.useInsertionEffect(() => {});
Dispatcher.useEffect(() => {});
Dispatcher.useImperativeHandle(undefined, () => null);
Dispatcher.useDebugValue(null);
Dispatcher.useCallback(() => {});
Dispatcher.useMemo(() => null);
if (typeof Dispatcher.useMemoCache === 'function') {
// This type check is for Flow only.
Dispatcher.useMemoCache(0);
}

When i was trying to debug this problem i notice that here they didn't call the useSyncExternalStore and useTransition from Dispatcher so I add the line Dispatcher.useSyncExternalStore(() => {}, () => {}, () => {}) above the line Dispatcher.useMemo(() => null) in above source code, and it worked fine. I don't know if this was intentionally or it a mistake.

@hoxyq
Copy link
Contributor

hoxyq commented Feb 20, 2024

function getPrimitiveStackCache(): Map<string, Array<any>> {
// This initializes a cache of all primitive hooks so that the top
// most stack frames added by calling the primitive hook can be removed.
if (primitiveStackCache === null) {
const cache = new Map<string, Array<any>>();
let readHookLog;
try {
// Use all hooks here to add them to the hook log.
Dispatcher.useContext(({_currentValue: null}: any));
Dispatcher.useState(null);
Dispatcher.useReducer((s: mixed, a: mixed) => s, null);
Dispatcher.useRef(null);
if (typeof Dispatcher.useCacheRefresh === 'function') {
// This type check is for Flow only.
Dispatcher.useCacheRefresh();
}
Dispatcher.useLayoutEffect(() => {});
Dispatcher.useInsertionEffect(() => {});
Dispatcher.useEffect(() => {});
Dispatcher.useImperativeHandle(undefined, () => null);
Dispatcher.useDebugValue(null);
Dispatcher.useCallback(() => {});
Dispatcher.useMemo(() => null);
if (typeof Dispatcher.useMemoCache === 'function') {
// This type check is for Flow only.
Dispatcher.useMemoCache(0);
}

When i was trying to debug this problem i notice that here they didn't call the useSyncExternalStore and useTransition from Dispatcher so I add the line Dispatcher.useSyncExternalStore(() => {}, () => {}, () => {}) above the line Dispatcher.useMemo(() => null) in above source code, and it worked fine. I don't know if this was intentionally or it a mistake.

Sorry for a late reply. Yes, this is required for generating stack frames of primitives, so this could be the root cause of why RDT can't find useSyncExternalStore hook.

@jamesbvaughan Thanks for providing the repro! Is someone interested in testing this solution against the repro and opening a PR?

@jamesbvaughan
Copy link
Contributor Author

@hoxyq I can try to put together a PR this week.

@jamesbvaughan
Copy link
Contributor Author

I've put put a PR: #28399

hoxyq pushed a commit that referenced this issue Feb 22, 2024
…28399)

<!--
  Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes. Please
provide enough information so that others can review your pull request.
The three fields below are mandatory.

Before submitting a pull request, please make sure the following is
done:

1. Fork [the repository](https://github.com/facebook/react) and create
your branch from `main`.
  2. Run `yarn` in the repository root.
3. If you've fixed a bug or added code that should be tested, add tests!
4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch
TestName` is helpful in development.
5. Run `yarn test --prod` to test in the production environment. It
supports the same options as `yarn test`.
6. If you need a debugger, run `yarn test --debug --watch TestName`,
open `chrome://inspect`, and press "Inspect".
7. Format your code with
[prettier](https://github.com/prettier/prettier) (`yarn prettier`).
8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only
check changed files.
  9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`).
  10. If you haven't already, complete the CLA.

Learn more about contributing:
https://reactjs.org/docs/how-to-contribute.html
-->

## Summary

<!--
Explain the **motivation** for making this change. What existing problem
does the pull request solve?
-->

This solves the problem of the devtools extension failing to parse hook
names for components that make use of `useSyncExternalStore` or
`useTransition`.

See #27889 

## How did you test this change?

<!--
Demonstrate the code is solid. Example: The exact commands you ran and
their output, screenshots / videos if the pull request changes the user
interface.
How exactly did you verify that your PR solves the issue you wanted to
solve?
  If you leave this empty, your PR will very likely be closed.
-->

I tested this against my own codebases and against the example repro
project that I linked in #27889.

To test, I opened up the Components tab of the dev tools extension,
selected a component with hooks that make use of `useSyncExternalStore`
or `useTransition`, clicked the "parse hook names" magic wand button,
and observed that it now succeeds.
@kenlyon
Copy link

kenlyon commented Mar 14, 2024

@hoxyq Could you advise when the DevTools Firefox Extension will be updated? This issue affects me and it's good to see that it's been fixed but my browser is stuck on v5.0.0 of the extension from December.

@hoxyq
Copy link
Contributor

hoxyq commented Mar 14, 2024

@hoxyq Could you advise when the DevTools Firefox Extension will be updated? This issue affects me and it's good to see that it's been fixed but my browser is stuck on v5.0.0 of the extension from December.

I've uploaded v5.0.2 to Firefox store a week ago, its still pending review process.

EdisonVan pushed a commit to EdisonVan/react that referenced this issue Apr 15, 2024
…acebook#28399)

<!--
  Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes. Please
provide enough information so that others can review your pull request.
The three fields below are mandatory.

Before submitting a pull request, please make sure the following is
done:

1. Fork [the repository](https://github.com/facebook/react) and create
your branch from `main`.
  2. Run `yarn` in the repository root.
3. If you've fixed a bug or added code that should be tested, add tests!
4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch
TestName` is helpful in development.
5. Run `yarn test --prod` to test in the production environment. It
supports the same options as `yarn test`.
6. If you need a debugger, run `yarn test --debug --watch TestName`,
open `chrome://inspect`, and press "Inspect".
7. Format your code with
[prettier](https://github.com/prettier/prettier) (`yarn prettier`).
8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only
check changed files.
  9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`).
  10. If you haven't already, complete the CLA.

Learn more about contributing:
https://reactjs.org/docs/how-to-contribute.html
-->

## Summary

<!--
Explain the **motivation** for making this change. What existing problem
does the pull request solve?
-->

This solves the problem of the devtools extension failing to parse hook
names for components that make use of `useSyncExternalStore` or
`useTransition`.

See facebook#27889 

## How did you test this change?

<!--
Demonstrate the code is solid. Example: The exact commands you ran and
their output, screenshots / videos if the pull request changes the user
interface.
How exactly did you verify that your PR solves the issue you wanted to
solve?
  If you leave this empty, your PR will very likely be closed.
-->

I tested this against my own codebases and against the example repro
project that I linked in facebook#27889.

To test, I opened up the Components tab of the dev tools extension,
selected a component with hooks that make use of `useSyncExternalStore`
or `useTransition`, clicked the "parse hook names" magic wand button,
and observed that it now succeeds.
bigfootjon pushed a commit that referenced this issue Apr 18, 2024
…28399)

<!--
  Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes. Please
provide enough information so that others can review your pull request.
The three fields below are mandatory.

Before submitting a pull request, please make sure the following is
done:

1. Fork [the repository](https://github.com/facebook/react) and create
your branch from `main`.
  2. Run `yarn` in the repository root.
3. If you've fixed a bug or added code that should be tested, add tests!
4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch
TestName` is helpful in development.
5. Run `yarn test --prod` to test in the production environment. It
supports the same options as `yarn test`.
6. If you need a debugger, run `yarn test --debug --watch TestName`,
open `chrome://inspect`, and press "Inspect".
7. Format your code with
[prettier](https://github.com/prettier/prettier) (`yarn prettier`).
8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only
check changed files.
  9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`).
  10. If you haven't already, complete the CLA.

Learn more about contributing:
https://reactjs.org/docs/how-to-contribute.html
-->

## Summary

<!--
Explain the **motivation** for making this change. What existing problem
does the pull request solve?
-->

This solves the problem of the devtools extension failing to parse hook
names for components that make use of `useSyncExternalStore` or
`useTransition`.

See #27889

## How did you test this change?

<!--
Demonstrate the code is solid. Example: The exact commands you ran and
their output, screenshots / videos if the pull request changes the user
interface.
How exactly did you verify that your PR solves the issue you wanted to
solve?
  If you leave this empty, your PR will very likely be closed.
-->

I tested this against my own codebases and against the example repro
project that I linked in #27889.

To test, I opened up the Components tab of the dev tools extension,
selected a component with hooks that make use of `useSyncExternalStore`
or `useTransition`, clicked the "parse hook names" magic wand button,
and observed that it now succeeds.

DiffTrain build for commit 47beb96.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Developer Tools Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Type: Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants