-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
Devtools: Add support for useFormStatus #28413
Conversation
Comparing: 4f5c812...de67894 Critical size changesIncludes critical production bundles, as well as any change greater than 2%: Significant size changesIncludes any change greater than 0.2%: Expand to show
|
26474b8
to
874faf9
Compare
874faf9
to
f0d6592
Compare
hookSource: { | ||
columnNumber: 0, | ||
fileName: '**', | ||
functionName: 'Object.useFormStatus', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the build tests in prod this comes out as Object.useHostTransitionStatus [as useFormStatus]
: https://app.circleci.com/pipelines/github/facebook/react/50936/workflows/b211c172-f33c-4e9f-806e-0eec46dcd88c/jobs/796848?invite=true#step-106-1107_84
This trips up debug-tools: all subsequent hooks (even built-in ones) are consider sub hooks.
The source map is not adding the [as ...]
since even without a source map I get "functionName": "Object.ia [as useFormStatus]",
. Either way, the mapping from ia
to useHostTransitionStatus
is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The [as ...]
comes from Node:
const Dispatcher = {
useFormStatus: function ia() {
console.log(new Error().stack);
},
};
Dispatcher.useFormStatus();
prints
Error
at Object.ia [as useFormStatus] (**)
the [as ...]
marker is removed when the function names match. Ideally it goes away once we fix the mapping or we can just ignore it once the mapping is fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a Jest issue. When I run
const React = require("react");
const ReactDOM = require("react-dom");
const ReactDebugTools = require("react-debug-tools");
function Component() {
ReactDOM.useFormStatus();
React.useMemo(() => {}, []);
React.useMemo(() => {}, []);
return null;
}
console.log(JSON.stringify(ReactDebugTools.inspectHooks(Component), null, 2));
via
$ NODE_ENV=production node --enable-source-maps test.js
[
{
"id": null,
"isStateEditable": false,
"name": "FormStatus",
"value": null,
"subHooks": [
{
"id": null,
"isStateEditable": false,
"name": "HostTransitionStatus",
"value": null,
"subHooks": [],
"debugInfo": null,
"hookSource": {
"lineNumber": 139,
"functionName": "useFormStatus",
"fileName": "/Users/sebastian.silbermann/react-debug-tools-useFormStatus/node_modules/react-dom/cjs/react-dom.production.js",
"columnNumber": 31
}
}
],
"debugInfo": null,
"hookSource": {
"lineNumber": 6,
"columnNumber": 12,
"functionName": "Component",
"fileName": "/Users/sebastian.silbermann/react-debug-tools-useFormStatus/test.js"
}
},
{
"id": 0,
"isStateEditable": false,
"name": "Memo",
"subHooks": [],
"debugInfo": null,
"hookSource": {
"lineNumber": 7,
"functionName": "Component",
"fileName": "/Users/sebastian.silbermann/react-debug-tools-useFormStatus/test.js",
"columnNumber": 9
}
},
{
"id": 1,
"isStateEditable": false,
"name": "Memo",
"subHooks": [],
"debugInfo": null,
"hookSource": {
"lineNumber": 8,
"functionName": "Component",
"fileName": "/Users/sebastian.silbermann/react-debug-tools-useFormStatus/test.js",
"columnNumber": 9
}
}
]
I get the correct output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't work in a browser either though:
-- https://react-devtools-hook-inspection-experimental.vercel.app/
160103d
to
88ccaec
Compare
// may not be the primitive. Likewise the primitive can have fewer stack frames | ||
// such as when a call to useState got inlined to use dispatcher.useState. | ||
// may not be the primitive. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how this can happen since the dispatcher is private. Without the "wrapper -> dispatcher" assumption I'm out of ideas on how to enable arbitrary wrapper names like useHostTransitionStatus
is trying to enable.
08a5fda
to
127b116
Compare
127b116
to
324c04a
Compare
6d47658
to
8a66fbd
Compare
43ed39e
to
4b0179b
Compare
4b0179b
to
dab9aca
Compare
wrapperNames: [ | ||
// react-dom | ||
'FormStatus', | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we actually need to support renderer-specific hooks. Same as for hooks from core, we relying on the hook name, but at least we have one core. Here, I am assuming, if we had React Native renderer, we would also treat a hook named useFormStatus
as primitive React hook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can double check but I don't think so since we only look one frame above the dispatcher which is always a core hook. So even if you would have a custom useFormStatus
hook, the stacktrace would consist of
dispatcher.useSomeDispatcherHook
React.useSomeReactHook
useFormstatus
Even if RN would implement have a useFormStatus
hook that's not wrapping useHostTransitionStatus
, we wouldn't mix them up with this implementation since we only expect useFormStatus
as a wrapper of useHostTransitionStatus
.
The whole wrapperNames
concept seems unnecessary though in hindsight. If we keep the assumption that a dispatcher method is wrapped by a hook and no intermediate wrappers, we could just immediately use the wrapper name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test to check we don't confuse hooks. @hoxyq let me know if that was the scenario you were talking about.
Also removed wrapperNames
since we don't need that concept.
dab9aca
to
bf7a1a0
Compare
just look one frame above
bf7a1a0
to
274e1cc
Compare
if ( | ||
i < hookStack.length - 1 && | ||
isReactWrapper(hookStack[i].functionName, hook.dispatcherHookName) | ||
) { | ||
i++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes
if ( | ||
i < hookStack.length - 1 && | ||
isReactWrapper(hookStack[i].functionName, hook.dispatcherHookName) | ||
) { | ||
i++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to skip 2 frames here, but I would also keep i < hookStack.length - 1
check.
Is it guaranteed that we will always have 2 frames from react?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of a case where we wouldn't have two frames. That would mean the dispatcher call is inlined into usercode. Maybe this can happen but at that point we'll never be able to recover the original React hook name. I'll add a check regardless since it's better to display the dispatcher name than crashing.
Summary: This sync includes the changes from: - D56103750 - [TODO] A shim for SECRET_INTERNALS This sync includes the following changes: - **[b5e5ce8e0](facebook/react@b5e5ce8e0 )**: Update ReactNativeTypes for root options (part 2) ([#28857](facebook/react#28857)) //<Ricky>// - **[da6ba53b1](facebook/react@da6ba53b1 )**: [UMD] Remove umd builds ([#28735](facebook/react#28735)) //<Josh Story>// - **[0c245df1d](facebook/react@0c245df1d )**: Complete the typo fix ([#28856](facebook/react#28856)) //<Sebastian Silbermann>// - **[f82051d7a](facebook/react@f82051d7a )**: console test utils fix: match entire string, not just first letter ([#28855](facebook/react#28855)) //<Andrew Clark>// - **[4ca20fd36](facebook/react@4ca20fd36 )**: Test top level fragment inside lazy semantics ([#28852](facebook/react#28852)) //<Sebastian Markbåge>// - **[c0cf7c696](facebook/react@c0cf7c696 )**: Promote ASYNC_ITERATOR symbol to React Symbols ([#28851](facebook/react#28851)) //<Sebastian Markbåge>// - **[657428a9e](facebook/react@657428a9e )**: Add ReactNativeTypes for root options ([#28850](facebook/react#28850)) //<Ricky>// - **[7909d8eab](facebook/react@7909d8eab )**: [Flight] Encode ReadableStream and AsyncIterables ([#28847](facebook/react#28847)) //<Sebastian Markbåge>// - **[13eb61d05](facebook/react@13eb61d05 )**: Move enableUseDeferredValueInitialArg to canary ([#28818](facebook/react#28818)) //<Andrew Clark>// - **[8afa144bd](facebook/react@8afa144bd )**: Enable flag disableClientCache ([#28846](facebook/react#28846)) //<Jan Kassens>// - **[734956ace](facebook/react@734956ace )**: Devtools: Add support for useFormStatus ([#28413](facebook/react#28413)) //<Sebastian Silbermann>// - **[17e920c00](facebook/react@17e920c00 )**: [Flight Reply] Encode Typed Arrays and Blobs ([#28819](facebook/react#28819)) //<Sebastian Markbåge>// - **[0347fcd00](facebook/react@0347fcd00 )**: Add on(Caught|Uncaught|Recoverable) opts to RN ([#28836](facebook/react#28836)) //<Ricky>// - **[c113503ad](facebook/react@c113503ad )**: Flush direct streams in Bun ([#28837](facebook/react#28837)) //<Kenta Iwasaki>// - **[9defcd56b](facebook/react@9defcd56b )**: Remove redundant props assign ([#28829](facebook/react#28829)) //<Sebastian Silbermann>// - **[ed4023603](facebook/react@ed4023603 )**: Fix mistaken "react-server" condition ([#28835](facebook/react#28835)) //<Sebastian Markbåge>// - **[c8a035036](facebook/react@c8a035036 )**: [Fizz] hoistables should never flush before the preamble ([#28802](facebook/react#28802)) //<Josh Story>// - **[4f5c812a3](facebook/react@4f5c812a3 )**: DevTools: Rely on sourcemaps to compute hook name of built-in hooks in newer versions ([#28593](facebook/react#28593)) //<Sebastian Silbermann>// - **[435415962](facebook/react@435415962 )**: Backwards compatibility for string refs on WWW ([#28826](facebook/react#28826)) //<Jack Pope>// - **[608edcc90](facebook/react@608edcc90 )**: [tests] add `assertConsole<method>Dev` helpers ([#28732](facebook/react#28732)) //<Ricky>// - **[da69b6af9](facebook/react@da69b6af9 )**: ReactDOM.requestFormReset ([#28809](facebook/react#28809)) //<Andrew Clark>// - **[374b5d26c](facebook/react@374b5d26c )**: Scaffolding for requestFormReset API ([#28808](facebook/react#28808)) //<Andrew Clark>// - **[41950d14a](facebook/react@41950d14a )**: Automatically reset forms after action finishes ([#28804](facebook/react#28804)) //<Andrew Clark>// - **[dc6a7e01e](facebook/react@dc6a7e01e )**: [Float] Don't preload images inside `<noscript>` ([#28815](facebook/react#28815)) //<Josh Story>// - **[3f947b1b4](facebook/react@3f947b1b4 )**: [tests] Assert scheduler log empty in internalAct ([#28737](facebook/react#28737)) //<Ricky>// - **[bf09089f6](facebook/react@bf09089f6 )**: Remove Scheduler.log from ReactSuspenseFuzz-test ([#28812](facebook/react#28812)) //<Ricky>// - **[84cb3b4cb](facebook/react@84cb3b4cb )**: Hardcode disableIEWorkarounds for www ([#28811](facebook/react#28811)) //<Ricky>// - **[2243b40ab](facebook/react@2243b40ab )**: [tests] assertLog before act in useEffectEvent ([#28763](facebook/react#28763)) //<Ricky>// - **[dfc64c6e3](facebook/react@dfc64c6e3 )**: [tests] assertLog before act in ReactUse ([#28762](facebook/react#28762)) //<Ricky>// - **[42eff4bc7](facebook/react@42eff4bc7 )**: [tests] Fix assertions not flushed before act ([#28745](facebook/react#28745)) //<Ricky>// - **[ed3c65caf](facebook/react@ed3c65caf )**: Warn if outdated JSX transform is detected ([#28781](facebook/react#28781)) //<Andrew Clark>// - **[3f9e237a2](facebook/react@3f9e237a2 )**: Fix: Suspend while recovering from hydration error ([#28800](facebook/react#28800)) //<Andrew Clark>// - **[7f5d25e23](facebook/react@7f5d25e23 )**: Fix cloneElement using string ref w no owner ([#28797](facebook/react#28797)) //<Joseph Savona>// - **[bf40b0244](facebook/react@bf40b0244 )**: [Fizz] Stop publishing external-runtime to stable channel ([#28796](facebook/react#28796)) //<Josh Story>// - **[7f93cb41c](facebook/react@7f93cb41c )**: [DOM] Infer react-server entries bundles if not explicitly configured ([#28795](facebook/react#28795)) //<Josh Story>// - **[f61316535](facebook/react@f61316535 )**: Rename SECRET INTERNALS to `__CLIENT_INTERNALS_DO_NOT_USE_OR_WARN_USERS_THEY_CANNOT_UPGRADE` ([#28789](facebook/react#28789)) //<Sebastian Markbåge>// - **[9644d206e](facebook/react@9644d206e )**: Soften useFormState warning ([#28788](facebook/react#28788)) //<Ricky>// - **[c771016e1](facebook/react@c771016e1 )**: Rename The Secret Export of Server Internals ([#28786](facebook/react#28786)) //<Sebastian Markbåge>// - **[d50323eb8](facebook/react@d50323eb8 )**: Flatten ReactSharedInternals ([#28783](facebook/react#28783)) //<Sebastian Markbåge>// - **[f62cf8c62](facebook/react@f62cf8c62 )**: [Float] treat `props.async` in Float consistent with the rest of react-dom ([#26760](facebook/react#26760)) //<Josh Story>// - **[dfd3d5af8](facebook/react@dfd3d5af8 )**: Add support for transition{run,start,cancel} events ([#27345](facebook/react#27345)) //<Hugo Sales>// - **[1f8327f83](facebook/react@1f8327f83 )**: [Fiber] Use real event priority for hydration scheduling ([#28765](facebook/react#28765)) //<Josh Story>// - **[97c90ed88](facebook/react@97c90ed88 )**: [DOM] Shrink ReactDOMCurrentDispatcher method names ([#28770](facebook/react#28770)) //<Josh Story>// - **[9007fdc8f](facebook/react@9007fdc8f )**: [DOM] Shrink ReactDOMSharedInternals source representation ([#28771](facebook/react#28771)) //<Josh Story>// - **[14f50ad15](facebook/react@14f50ad15 )**: [Flight] Allow lazily resolving outlined models ([#28780](facebook/react#28780)) //<Sebastian Markbåge>// - **[4c12339ce](facebook/react@4c12339ce )**: [DOM] move `flushSync` out of the reconciler ([#28500](facebook/react#28500)) //<Josh Story>// - **[8e1462e8c](facebook/react@8e1462e8c )**: [Fiber] Move updatePriority tracking to renderers ([#28751](facebook/react#28751)) //<Josh Story>// - **[0b3b8a6a3](facebook/react@0b3b8a6a3 )**: jsx: Remove unnecessary hasOwnProperty check ([#28775](facebook/react#28775)) //<Andrew Clark>// - **[2acfb7b60](facebook/react@2acfb7b60 )**: [Flight] Support FormData from Server to Client ([#28754](facebook/react#28754)) //<Sebastian Markbåge>// - **[d1547defe](facebook/react@d1547defe )**: Fast JSX: Don't clone props object ([#28768](facebook/react#28768)) //<Andrew Clark>// - **[bfd8da807](facebook/react@bfd8da807 )**: Make class prop resolution faster ([#28766](facebook/react#28766)) //<Andrew Clark>// - **[cbb6f2b54](facebook/react@cbb6f2b54 )**: [Flight] Support Blobs from Server to Client ([#28755](facebook/react#28755)) //<Sebastian Markbåge>// - **[f33a6b69c](facebook/react@f33a6b69c )**: Track Owner for Server Components in DEV ([#28753](facebook/react#28753)) //<Sebastian Markbåge>// - **[e3ebcd54b](facebook/react@e3ebcd54b )**: Move string ref coercion to JSX runtime ([#28473](facebook/react#28473)) //<Andrew Clark>// - **[fd0da3eef](facebook/react@fd0da3eef )**: Remove _owner field from JSX elements in prod if string refs are disabled ([#28739](facebook/react#28739)) //<Sebastian Markbåge>// Changelog: [General][Changed] - React Native sync for revisions 48b4ecc...b5e5ce8 jest_e2e[run_all_tests] bypass-github-export-checks Reviewed By: kassens Differential Revision: D56251607 fbshipit-source-id: e16db2fa101fc7ed1e009158c76388206beabd5f
Full list of changes (not a public changelog): * fix[react-devtools/ci]: fix configurations for e2e testing ([hoxyq](https://github.com/hoxyq) in [#29016](#29016)) * feat[react-devtools]: display forget badge for components in profiling session ([hoxyq](https://github.com/hoxyq) in [#29014](#29014)) * fix[react-devtools]: add backwards compat with legacy element type symbol ([hoxyq](https://github.com/hoxyq) in [#28982](#28982)) * Expose "view source" options to Fusebox integration ([motiz88](https://github.com/motiz88) in [#28973](#28973)) * Enable inspected element context menu in Fusebox ([motiz88](https://github.com/motiz88) in [#28972](#28972)) * Check in `frontend.d.ts` for react-devtools-fusebox, include in build output ([motiz88](https://github.com/motiz88) in [#28970](#28970)) * Devtools: Fix build-for-devtools ([eps1lon](https://github.com/eps1lon) in [#28976](#28976)) * Move useMemoCache hook to react/compiler-runtime ([kassens](https://github.com/kassens) in [#28954](#28954)) * warn -> error for Test Renderer deprecation ([acdlite](https://github.com/acdlite) in [#28904](#28904)) * [react-dom] move all client code to `react-dom/client` ([gnoff](https://github.com/gnoff) in [#28271](#28271)) * Rename the react.element symbol to react.transitional.element ([sebmarkbage](https://github.com/sebmarkbage) in [#28813](#28813)) * Rename Forget badge ([jbonta](https://github.com/jbonta) in [#28858](#28858)) * Devtools: Add support for useFormStatus ([eps1lon](https://github.com/eps1lon) in [#28413](#28413))
Stack:
useFormStatus
returnsNotPendingTransition
#28728Unfortunately, the initial value will be incorrect since that value specified in the Fiber config to which react-debug-tools has no access until we land #28728.
Test plan
yarn test ReactHooksInspectionIntegrationDOM
FormStatus
) in the shellScreen.Recording.2024-03-05.at.11.06.16.mov