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.

Sign up for GitHub

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

Jump to bottom

[Fix] Simplify pendingState type in useActionState #28942

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+1 −1
Open

[Fix] Simplify pendingState type in useActionState #28942

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactFiberHooks.js
View file
Open in desktop
Expand Up @@ -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));
Copy link
Collaborator

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?

Copy link
Author

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.

Copy link
Collaborator

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?

Copy link
Author

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:

react/packages/react-reconciler/src/ReactFiberHooks.js

Line 2033 in 190cc99

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.

const setPendingState: boolean => void = (dispatchOptimisticSetState.bind(
null,
currentlyRenderingFiber,
Expand Down