[DevTools] Add support for useMemoCache #26696
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
poteto
force-pushed
the
lt/devtools-uMC
branch
2 times, most recently
from
0266d1f
to
ad0bd14
Compare
|
cc @mondaychen, for some reason I couldn't add you as a reviewer |
|
No sure why the test is failing on CI. It works OK on my local machine. |
There was a problem hiding this comment.
It may appear that react-debug-tools is a separate package from react-devtools-* packages, allowing you to safely use shared/* within it. However, the truth is that it has always been an integral part of the React DevTools backend, which is designed to be compatible with any React version after v15.
If I had been able to complete my DevTools backend re-architecture project, this issue would be less concerning since we will have lockstep between React and DevTools. 🥲
At the moment, I don't have an optimal solution. Perhaps it's acceptable assuming the implementation of useMemoCache remains unchanged in the near future.
| @@ -15,6 +15,7 @@ import type { | |||
| ReactProviderType, | |||
| StartTransitionOptions, | |||
| } from 'shared/ReactTypes'; | |||
| import {REACT_MEMO_CACHE_SENTINEL} from 'shared/ReactSymbols'; | |||
There was a problem hiding this comment.
This might cause compatibility issues in the future. See packages/react-devtools-shared/src/backend/ReactSymbols.js
Also, this is not a type, so should not be in the middle of other import type lines
|
Also @poteto it looks like I'm not longer authorized to approve or request changes on PRs here 😢 |
| data = new Array(size); | ||
| for (let i = 0; i < size; i++) { | ||
| data[i] = REACT_MEMO_CACHE_SENTINEL; | ||
| } |
There was a problem hiding this comment.
nit: can this be simplified with Array.fill?
There was a problem hiding this comment.
Thanks! We ran some benchmarks on this before: #25465 (comment)
|
@mondaychen @hoxyq any idea why this test variant would have different results? |
|
@poteto any reason you need to use |
No, I changed it already but the fact is still that this particular test variant has a different output than all the other variants, and I'm not sure why |
I think there is a problem with this function: react/packages/react-debug-tools/src/ReactDebugHooks.js Lines 457 to 469 in 919620b Here This is where the things are starting to break. When we build hooks tree, at some point we are parsing the stack and if its not empty, then we are parsing custom hook names: react/packages/react-debug-tools/src/ReactDebugHooks.js Lines 542 to 564 in 919620b That's why we receiving |
useMemoCache wasn't previously supported in the DevTools, so any attempt to inspect a component using the hook would result in a `dispatcher.useMemoCache is not a function (it is undefined)` error.
|
@hoxyq awesome, thanks for the pointer! I fixed the test by aliasing the unstable_ prefixed name to one without, rather than change anything in that code you linked. This is more correct for user code (eg lint rules would probably be written for the unprefixed hook name) and we'll eventually drop the unstable prefix anyway so there's nothing "wrong" about the current implementation not supporting unstable imo. |
We don't need this symbol allocated unless we're handling a useMemoCache call
…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.
useMemoCache wasn't previously supported in the DevTools, so any attempt to inspect a component using the hook would result in a `dispatcher.useMemoCache is not a function (it is undefined)` error.
…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.
useMemoCache wasn't previously supported in the DevTools, so any attempt to inspect a component using the hook would result in a `dispatcher.useMemoCache is not a function (it is undefined)` error. DiffTrain build for commit 25b99ef.
useMemoCache wasn't previously supported in the DevTools, so any attempt to inspect a component using the hook would result in a
dispatcher.useMemoCache is not a function (it is undefined)error.