react icon indicating copy to clipboard operation
react copied to clipboard

[Fix] Simplify pendingState type in useActionState

Open yongsk0066 opened this issue 2 months ago • 3 comments

Summary

This pull request simplifies the type definition of pendingState in the useActionState hook by changing it from Thenable<boolean> | boolean to just boolean. The current implementation of useActionState defines the type of pendingState as Thenable<boolean> | boolean. However, upon closer inspection of the code, it appears that pendingState is always set to a boolean value, either directly or via the setPendingState function, which only accepts a boolean argument.

pendingStateHook = mountStateImpl((false: Thenable<boolean> | boolean));
const setPendingState: boolean => void = (dispatchOptimisticSetState.bind(
  null,
  currentlyRenderingFiber,
  false,
  ((pendingStateHook.queue: any): UpdateQueue<
    S | Awaited<S>,
    S | Awaited<S>,
  >),
): any);

In the dispatchActionState function, setPendingState is always called with a boolean value:

setPendingState(true);

Therefore, defining pendingState as Thenable<boolean> | boolean seems unnecessary, as it's never actually set to a thenable value. This pull request proposes to simplify the type to boolean, which more accurately reflects how pendingState 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 of pendingState throughout the useActionState 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:

function updateTransition(): [
  boolean,
  (callback: () => void, options?: StartTransitionOptions) => void,
] {
  const [booleanOrThenable] = updateState(false);
  const hook = updateWorkInProgressHook();
  const start = hook.memoizedState;
  const isPending =
    typeof booleanOrThenable === 'boolean'
      ? booleanOrThenable
      : // This will suspend until the async action scope has finished.
        useThenable(booleanOrThenable);
  return [isPending, start];
}

Here, booleanOrThenable is the result of updateState(false), and its type is Thenable<boolean> | boolean. The value of isPending depends on the type of booleanOrThenable:

  • If booleanOrThenable is a boolean,isPending directly uses that value.
  • If booleanOrThenable is a thenable object, isPending is assigned the result of processing the thenable with useThenable.

Therefore, in useTransition, the Thenable<boolean> | boolean type is appropriate for isPending, as it can hold a thenable value. In contrast, pendingState in useActionState is always used as a boolean value, so we can simplify its type to boolean.

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.

yongsk0066 avatar Apr 28 '24 04:04 yongsk0066