Fixing Transitions

[webdevcody]

There was still a little bug related to transitions. Basically the useServerAction would invoke onSuccess before the server action was done running revalidatePath or redirect. This refactoring includes

• removing the executing boolean since a transition already tracks the action state
• using an effect to listen for the isTransitioning flag to know exactly when we should be calling onSuccess, onError, and resolving the original promise with the results
• refactoring the approach for resolving the promise by storing it in a ref and resolving it in the effect

if I get time, I want to add some type of testing to verify this bug won't happen again. The logic for this useServerAction is a bit complex, and I'm worried it's going to start becoming bug food as things are refactored or changed. Unforatnely I think this type of testing would require playwright or cypress because the testing would require verifying the hooks work with the underlying revalidatePath and redirect methods in combination to delays in RSCs refetching data.

[changeset-bot[bot]]

⚠️ No Changeset found

Latest commit: 4859987343fd6b66f4ab9c3e0723c218efb5f1db

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
zsa	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 5, 2024 6:23pm

[IdoPesok]

Good catch, appreciate the help with all this transition stuff. Wish startTransition accepted a callback or something so we didn't need this useEffect stuff but seems like this is what we got to do.

Concur with the testing -- @WesternConcrete got client side tests going on #75 but it seems like, as you said, testing redirect might be out of Jest's scope.

The one thing I am thinking about is how this affects the retry option. I originally kept both isExecuting and isTransitioning originally so that, when retrying, isExecuting would still be true. It seems like now the status is no longer pending when it is retrying. Here is a snippet you can try:

  const { isPending, execute, data, error, isError } = useServerAction(
    incrementNumberAction,
    {
      retry: {
        maxAttempts: 3,
        delay: 1000,
      },
    }
  )

Happy to merge this once the retry functionality is stable. The changes look good and solve the onSuccess bug.

[webdevcody]

@IdoPesok double check when you get a chance, I tested on my own app with retries and without, it seems to continue to show the loader until after all the retries finish

[IdoPesok]

Added test cases for retrying in #92 , closing this PR as these commits are in there.