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/useTransition]: don't check for dispatch property when determining if hook is stateful #27365

Merged

Conversation

hoxyq
Copy link
Contributor

@hoxyq hoxyq commented Sep 12, 2023

#26740 introduced regression: React DevTools doesn't record updates for useTransition hook. I can add more details about things on DevTools side, if needed.

The root cause is

// We're going to cheat and intentionally not create a bound dispatch
// method, because we can call it directly in startTransition.
dispatch: (null: any),

React DevTools expects dispatch to be present for stateful hooks that can schedule an update -

// Detect the shape of useState() or useReducer()
// using the attributes that are unique to these hooks
// but also stable (e.g. not tied to current Lanes implementation)
const isStateOrReducer =
boundHasOwnProperty('pending') &&
boundHasOwnProperty('dispatch') &&
typeof queue.dispatch === 'function';

With these changes, we still call dispatch in startTransition, but also patch queue object with it, so that React DevTools can recognise useTransition as stateful hook that can schedule update.

I am not sure if this is the right approach to fix this, can we distinguish if startTransition was called from useTransition hook or as a standalone function?

@hoxyq hoxyq requested a review from acdlite September 12, 2023 15:36
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Sep 12, 2023
@acdlite
Copy link
Collaborator

acdlite commented Sep 20, 2023

This is the exact type of implementation detail leakage that bothers me so much about the current DevTools architecture. I'd rather DevTools check a version string and fork based on that.

@hoxyq
Copy link
Contributor Author

hoxyq commented Sep 20, 2023

This is the exact type of implementation detail leakage that bothers me so much about the current DevTools architecture. I'd rather DevTools check a version string and fork based on that.

Agree, but aren't there any other ways to understand if hook can schedule an update or if it can contain state, from reconciler perspective?

@hoxyq hoxyq force-pushed the fix/set-bound-dispatch-for-transition-hook branch from ae957ad to 038617d Compare September 21, 2023 19:00
@hoxyq hoxyq changed the title fix[devtools/useTransition]: set bound dispatch for useTransition queue fix[devtools/useTransition]: don't check for dispatch property when determining if hook is stateful Sep 21, 2023
@hoxyq
Copy link
Contributor Author

hoxyq commented Sep 21, 2023

Switched to checking just pending property for these hooks, as was discussed with @acdlite

@hoxyq hoxyq force-pushed the fix/set-bound-dispatch-for-transition-hook branch from 038617d to 2e28c42 Compare September 21, 2023 20:02
@hoxyq hoxyq merged commit 56b1447 into facebook:main Sep 21, 2023
36 checks passed
@hoxyq hoxyq deleted the fix/set-bound-dispatch-for-transition-hook branch September 21, 2023 20:26
hoxyq added a commit that referenced this pull request Sep 25, 2023
* refactor[devtools/extension]: refactored messaging logic across
different parts of the extension ([hoxyq](https://github.com/hoxyq) in
[#27417](#27417))
* fix[devtools/extension]: added a workaround for proxy content script
injection in firefox ([hoxyq](https://github.com/hoxyq) in
[#27375](#27375))
* fix[devtools/useTransition]: don't check for dispatch property when
determining if hook is stateful ([hoxyq](https://github.com/hoxyq) in
[#27365](#27365))
* feat[devtools/extension]: show disclaimer when page doesnt run react
and refactor react polling logic ([hoxyq](https://github.com/hoxyq) in
[#27373](#27373))
* feat:-Added a delete all filters action and added title to the add
filter a… ([Biki-das](https://github.com/Biki-das) in
[#27332](#27332))
* fix[devtools/extension]: unregister dynamically injected content
scripts instead of filtering ([hoxyq](https://github.com/hoxyq) in
[#27369](#27369))
* refactor[devtools/extension]: more stable element updates polling to
avoid timed out errors ([hoxyq](https://github.com/hoxyq) in
[#27357](#27357))
* feat[devtools/extension]: add dark theme for popup
([rakleed](https://github.com/rakleed) in
[#27330](#27330))
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…etermining if hook is stateful (facebook#27365)

facebook#26740 introduced regression:
React DevTools doesn't record updates for `useTransition` hook. I can
add more details about things on DevTools side, if needed.

The root cause is
https://github.com/facebook/react/blob/491aec5d6113ce5bae7c10966bc38a4a8fc091a8/packages/react-reconciler/src/ReactFiberHooks.js#L2728-L2730

React DevTools expects dispatch to be present for stateful hooks that
can schedule an update -
https://github.com/facebook/react/blob/2eed1328478e8c923fcb4e6abf5efbd9e1233402/packages/react-devtools-shared/src/backend/renderer.js#L1422-L1428

With these changes, we still call dispatch in `startTransition`, but
also patch `queue` object with it, so that React DevTools can recognise
`useTransition` as stateful hook that can schedule update.

I am not sure if this is the right approach to fix this, can we
distinguish if `startTransition` was called from `useTransition` hook or
as a standalone function?
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
* refactor[devtools/extension]: refactored messaging logic across
different parts of the extension ([hoxyq](https://github.com/hoxyq) in
[facebook#27417](facebook#27417))
* fix[devtools/extension]: added a workaround for proxy content script
injection in firefox ([hoxyq](https://github.com/hoxyq) in
[facebook#27375](facebook#27375))
* fix[devtools/useTransition]: don't check for dispatch property when
determining if hook is stateful ([hoxyq](https://github.com/hoxyq) in
[facebook#27365](facebook#27365))
* feat[devtools/extension]: show disclaimer when page doesnt run react
and refactor react polling logic ([hoxyq](https://github.com/hoxyq) in
[facebook#27373](facebook#27373))
* feat:-Added a delete all filters action and added title to the add
filter a… ([Biki-das](https://github.com/Biki-das) in
[facebook#27332](facebook#27332))
* fix[devtools/extension]: unregister dynamically injected content
scripts instead of filtering ([hoxyq](https://github.com/hoxyq) in
[facebook#27369](facebook#27369))
* refactor[devtools/extension]: more stable element updates polling to
avoid timed out errors ([hoxyq](https://github.com/hoxyq) in
[facebook#27357](facebook#27357))
* feat[devtools/extension]: add dark theme for popup
([rakleed](https://github.com/rakleed) in
[facebook#27330](facebook#27330))
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
…etermining if hook is stateful (#27365)

#26740 introduced regression:
React DevTools doesn't record updates for `useTransition` hook. I can
add more details about things on DevTools side, if needed.

The root cause is
https://github.com/facebook/react/blob/491aec5d6113ce5bae7c10966bc38a4a8fc091a8/packages/react-reconciler/src/ReactFiberHooks.js#L2728-L2730

React DevTools expects dispatch to be present for stateful hooks that
can schedule an update -
https://github.com/facebook/react/blob/2eed1328478e8c923fcb4e6abf5efbd9e1233402/packages/react-devtools-shared/src/backend/renderer.js#L1422-L1428

With these changes, we still call dispatch in `startTransition`, but
also patch `queue` object with it, so that React DevTools can recognise
`useTransition` as stateful hook that can schedule update.

I am not sure if this is the right approach to fix this, can we
distinguish if `startTransition` was called from `useTransition` hook or
as a standalone function?

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

3 participants