-
Notifications
You must be signed in to change notification settings - Fork 47.8k
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: Unwrap Promise in useFormState #28319
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -494,16 +494,50 @@ function useFormState<S, P>( | |
const hook = nextHook(); // FormState | ||
nextHook(); // ActionQueue | ||
let state; | ||
let debugInfo = null; | ||
if (hook !== null) { | ||
state = hook.memoizedState; | ||
const actionResult = hook.memoizedState; | ||
if ( | ||
typeof actionResult === 'object' && | ||
actionResult !== null && | ||
// $FlowFixMe[method-unbinding] | ||
typeof actionResult.then === 'function' | ||
) { | ||
const thenable: Thenable<Awaited<S>> = (actionResult: any); | ||
switch (thenable.status) { | ||
case 'fulfilled': { | ||
state = thenable.value; | ||
debugInfo = | ||
thenable._debugInfo === undefined ? null : thenable._debugInfo; | ||
break; | ||
} | ||
case 'rejected': { | ||
const rejectedError = thenable.reason; | ||
throw rejectedError; | ||
} | ||
default: | ||
// If this was an uncached Promise we have to abandon this attempt | ||
// but we can still emit anything up until this point. | ||
hookLog.push({ | ||
primitive: 'Unresolved', | ||
stackError: new Error(), | ||
value: thenable, | ||
debugInfo: | ||
thenable._debugInfo === undefined ? null : thenable._debugInfo, | ||
}); | ||
throw SuspenseException; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Followed the same pattern as #28297. Do we actually reach this with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even if instrumented, the promise could still be pending. |
||
} | ||
} else { | ||
state = (actionResult: any); | ||
} | ||
} else { | ||
state = initialState; | ||
} | ||
hookLog.push({ | ||
primitive: 'FormState', | ||
stackError: new Error(), | ||
value: state, | ||
debugInfo: null, | ||
debugInfo: debugInfo, | ||
}); | ||
return [state, (payload: P) => {}]; | ||
} | ||
|
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.
This needs a different name and you need to exhaust this case in the stack frame cache. See https://github.com/facebook/react/blob/main/packages/react-debug-tools/src/ReactDebugHooks.js#L111-L117
Otherwise this will generate the wrong stack frame information and so it'll generate a messed up tree.
Probably should add some tests for it.
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.
Did that just like use but it's not clear to me how I could test it. It was working fine in the fixture without the case as far as I could tell.
I guess I would need to simulate an update in
getPrimitiveStackCache
because we never actually calluseFormState
with a thenable?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.
Can it just use
FormState
? I don't understand the purpose of using different primitive names for the same 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.
It's annoying because this is used both for the key in the cache map and each one will have different stack traces. So it needs to be unique per stack. However, it's also used for the display name.
Which is why I gave it a unique primitive but then aliased it back:
https://github.com/facebook/react/blob/main/packages/react-debug-tools/src/ReactDebugHooks.js#L795
We could just make two separate fields for the name and the key potentially.
Since both your cases are in the same Hook you can also just unify them so that they have the same
new Error().stack
and ideally the samehookLog.push(...)
.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.
Unified stack error and hook log. I have to fight flow analysis more when unifying the hook log push more unless I tie up
value
anderror
into a single type like{ value: Awaited<S>, error: null } | { value: Thenable, error: mixed }
.