-
Notifications
You must be signed in to change notification settings - Fork 45.9k
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] Simplify pendingState type in useActionState #28942
base: main
Are you sure you want to change the base?
[Fix] Simplify pendingState type in useActionState #28942
Conversation
Hi @yongsk0066! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
Comparing: 190cc99...9da7559 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
|
@@ -2187,7 +2187,7 @@ function mountActionState<S, P>( | |||
|
|||
// Pending state. This is used to store the pending state of the action. | |||
// Tracked optimistically, like a transition pending state. | |||
const pendingStateHook = mountStateImpl((false: Thenable<boolean> | boolean)); | |||
const pendingStateHook = mountStateImpl((false: boolean)); |
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 previous code matches how we set pending throughout this file. Why is this the only place where it isn't 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.
I understand the desire for consistency, but adding the Thenable type to pendingState when it's not actually used as a Thenable value seems unnecessary. It could make the code harder to understand. I propose we prioritize clarity by matching the type definition to how pendingState is actually used.
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.
Is it not used like this in the other cases?
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 case of useTransition
, a thenable value is indeed passed through chainThenableValue
, as shown in the
code :
react/packages/react-reconciler/src/ReactFiberHooks.js
Lines 2897 to 2901 in 190cc99
const thenableForFinishedState = chainThenableValue( | |
thenable, | |
finishedState, | |
); | |
dispatchSetState(fiber, queue, (thenableForFinishedState: any)); |
const thenableForFinishedState = chainThenableValue(thenable, finishedState);
dispatchSetState(fiber, queue, (thenableForFinishedState: any));
However, in the useActionState
case, specifically in the dispatchActionState
function, setPendingState
is always called with a boolean value true
:
setPendingState(true); |
function runActionStateAction<S, P>(
//...
setPendingState(true);
In useActionState
, pendingState
is consistently treated as a boolean value, whereas in useTransition
, it has the flexibility to be either a boolean or a thenable.
Summary
This pull request simplifies the type definition of
pendingState
in theuseActionState
hook by changing it fromThenable<boolean> | boolean
to justboolean
.The current implementation of
useActionState
defines the type ofpendingState
asThenable<boolean> | boolean.
However, upon closer inspection of the code, it appears thatpendingState
is always set to a boolean value, either directly or via thesetPendingState
function, which only accepts a boolean argument.In the
dispatchActionState
function,setPendingState
is always called with a boolean value:Therefore, defining
pendingState
asThenable<boolean> | boolean
seems unnecessary, as it's never actually set to a thenable value. This pull request proposes to simplify the type toboolean
, which more accurately reflects howpendingState
is used in practice.This change improves code clarity and maintainability by removing the potentially confusing
Thenable
type when it's not needed. It also aligns the type definition with the actual usage ofpendingState
throughout theuseActionState
implementation.Comparison with
useTransition
While the code for pendingState in useActionState appears to have been developed with reference to the useTransition hook, it's important to note that these are two different cases that require different handling.
In useTransition,
isPending
can actually hold a thenable value, as evidenced by this code:Here,
booleanOrThenable
is the result ofupdateState(false)
, and its type isThenable<boolean> | boolean
. The value ofisPending
depends on the type ofbooleanOrThenable
:booleanOrThenable
is a boolean,isPending
directly uses that value.booleanOrThenable
is a thenable object,isPending
is assigned the result of processing the thenable withuseThenable
.Therefore, in
useTransition
, theThenable<boolean> | boolean
type is appropriate forisPending
, as it can hold a thenable value.In contrast,
pendingState
inuseActionState
is always used as a boolean value, so we can simplify its type toboolean
.How did you test this change?
To verify that this change doesn't introduce any regressions, I ran the existing test suite with
yarn test
. All tests passed without any issues, indicating that the updated type definition does not break any existing functionality.