[useFormState] Allow sync actions #27571
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: 960ed6e...e08ebb1 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
| @@ -76,7 +76,7 @@ export function useFormStatus(): FormStatus { | |||
| } | |||
|
|
|||
| export function useFormState<S, P>( | |||
| action: (S, P) => Promise<S>, | |||
| action: (S, P) => S | Promise<S>, | |||
There was a problem hiding this comment.
This is not quite right. In TypeScript I think this would be:
export function useFormState<S, P>(
action: (Awaited<S>, P) => S,
initialState: Awaited<S>,
permalink?: string,
): [Awaited<S>, (P) => void] {
So if S is a Promise, the state is the awaited result. If S is not a promise it is the same as S.
Is there some equivalent in Flow?
There was a problem hiding this comment.
there’s not one built in but I asked the Flow team about it and they showed me how to create an equivalent, just forgot to update the PR
There was a problem hiding this comment.
I can't speak for Flow, but in TypeScript the action type as chosen by Andrew is the idiomatic way of declaring a callback that can be sync or async, if I'm not mistaken. The usage of Awaited as proposed here would actually lead to a compiler error for async actions, as demonstrated in this TypeScript playground.
There was a problem hiding this comment.
If your intention with using Awaited is to disallow a promise being used as initial state, which the type did not prevent before but maybe should, a possible solution would be to use a combination of both proposed signatures:
function useFormState<S, P>(
action: (prevState: Awaited<S>, payload: P) => S | Promise<S>,
initialState: Awaited<S>,
permalink?: string,
): [Awaited<S>, (payload: P) => void];See also this playground with a comparison of all three solutions.
There was a problem hiding this comment.
Huh, I don't really understand why the S | Promise<S> thing is necessary. I'm just going to let @eps1lon figure this out in the DefinitelyTyped repo since that's the one that actually matters 😆 For our internal Flow types, I updated it to @sebmarkbage 's version (165424e), which mostly worked but I did hit a few "recursion limit exceeded" errors and ended up needing to resort to any in a few places anyway. But as long as the public ones are correct it's not that important.
acdlite
force-pushed
the
useformstate-allow-sync-action
branch
3 times, most recently
from
5bfbd56
to
2202499
Compare
| @@ -184,3 +184,13 @@ export type ReactFormState<S, ReferenceId> = [ | |||
| ReferenceId /* Server Reference ID */, | |||
| number /* number of bound arguments */, | |||
| ]; | |||
|
|
|||
| export type Awaited<T> = T extends null | void | |||
There was a problem hiding this comment.
Copied this from the TypeScript codebase
Updates useFormState to allow a sync function to be passed as an action. A form action is almost always async, because it needs to talk to the server. But since we support client-side actions, too, there's no reason we can't allow sync actions, too. I originally chose not to allow them to keep the implementation simpler but it's not really that much more complicated because we already support this for actions passed to startTransition. So now it's consistent: anywhere an action is accepted, a sync client function is a valid input.
acdlite
force-pushed
the
useformstate-allow-sync-action
branch
3 times, most recently
from
d6a6d45
to
9a604dc
Compare
Needed to bump this to get support for Flow's `extends` syntax.
Updates our internal Flow type for useFormState to allow for synchronous actions. I tried to make it match what the TypeScript equivalent would be, so although Flow does not include an Awaited type helper, I copied the one from TypeScript and added it to our internal types. Unfortunately this led to a few "Recursion limit exceeded" Flow errors in the Fiber implementation. I wasn't able to figure out whether these were legit type errors, or if the types were correct but the recursiveness of the types tripped some internal limit. But since this only affects the type coverage of our internal implementation and not the public API, I sprinkled in some `any`s and called it a day.
acdlite
force-pushed
the
useformstate-allow-sync-action
branch
from
9a604dc
to
e08ebb1
Compare
Updates useFormState to allow a sync function to be passed as an action. A form action is almost always async, because it needs to talk to the server. But since we support client-side actions, too, there's no reason we can't allow sync actions, too. I originally chose not to allow them to keep the implementation simpler but it's not really that much more complicated because we already support this for actions passed to startTransition. So now it's consistent: anywhere an action is accepted, a sync client function is a valid input. DiffTrain build for [77c4ac2](77c4ac2)
| permalink?: string, | ||
| ) => [S, (P) => void], | ||
| ) => [Awaited<S>, (P) => void], |
There was a problem hiding this comment.
As far as I can tell, we only need the Awaited in action and not initialState nor return value: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/67266/files
Let me know if I missed some scenarios that should be rejected at the type-level
There was a problem hiding this comment.
When I tested this last week, passing in a promise as initial state, I got this (rather misleading) error:
async/await is not yet supported in Client Components, only Server Components. This error is often caused by accidentally adding
'use client'to a module that was originally written for the server.
But the types also did not prevent this before DefinitelyTyped/DefinitelyTyped#67266.
There was a problem hiding this comment.
Ah never mind, it does give you a type error in your variant, so it's all fine I guess.
- facebook/react#27641 - facebook/react#27661 - facebook/react#27640 - facebook/react#27595 - facebook/react#27600 - facebook/react#27598 - facebook/react#27590 - facebook/react#27606 - facebook/react#27608 - facebook/react#27601 - facebook/react#27616 - facebook/react#27615 - facebook/react#27614 - facebook/react#27599 - facebook/react#27597 - facebook/react#27525 - facebook/react#27571
Updates React from 08a39539f to 2983249dd. ### React upstream changes - facebook/react#27641 - facebook/react#27661 - facebook/react#27640 - facebook/react#27595 - facebook/react#27600 - facebook/react#27598 - facebook/react#27590 - facebook/react#27606 - facebook/react#27608 - facebook/react#27601 - facebook/react#27616 - facebook/react#27615 - facebook/react#27614 - facebook/react#27599 - facebook/react#27597 - facebook/react#27525 - facebook/react#27571 Updates @types/react to 18.2.37 Updates @types/react-dom to 18.2.15
Updates useFormState to allow a sync function to be passed as an action. A form action is almost always async, because it needs to talk to the server. But since we support client-side actions, too, there's no reason we can't allow sync actions, too. I originally chose not to allow them to keep the implementation simpler but it's not really that much more complicated because we already support this for actions passed to startTransition. So now it's consistent: anywhere an action is accepted, a sync client function is a valid input.
Updates useFormState to allow a sync function to be passed as an action. A form action is almost always async, because it needs to talk to the server. But since we support client-side actions, too, there's no reason we can't allow sync actions, too. I originally chose not to allow them to keep the implementation simpler but it's not really that much more complicated because we already support this for actions passed to startTransition. So now it's consistent: anywhere an action is accepted, a sync client function is a valid input. DiffTrain build for commit 77c4ac2.
Updates useFormState to allow a sync function to be passed as an action.
A form action is almost always async, because it needs to talk to the server. But since we support client-side actions, too, there's no reason we can't allow sync actions, too.
I originally chose not to allow them to keep the implementation simpler but it's not really that much more complicated because we already support this for actions passed to startTransition. So now it's consistent: anywhere an action is accepted, a sync client function is a valid input.