Closed andresgutgon closed 2 days ago
Latest commit: 45666516ce00e03a3fbe4bf95778f7989a201f0f
The changes in this PR will be included in the next version bump.
Not sure what this means? Click here to learn what changesets are.
Click here if you're a maintainer who wants to add another changeset to this PR
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 29, 2024 8:53am |
I just experimented with react query mutations and seems like they also do not keep old data/error while the mutation is pending. Furthermore, SWR does not return old data. But your layout shift in the before video is understandably annoying and the after looks much better. I'm curious if this should be an opt in thing, and what are possible negative side effects that may result from keeping errors/data while pending.
what are possible negative side effects that may result from keeping errors/data while pending.
This is what we need to find out. Can you think on any? Also keep in mind this is a validation library I think keeping errors while running is the expected behaviour
I think the negative side is the types will be lying. When status is pending, error can be undefined or the error type. If we update the types though to tell the truth, we lose the narrowing of types using isError
, isPending
, and isSuccess
. I assume this is why react query also does not keep errors while is pending is true and every status has its respective shape.
A possible solution that I am leaning towards is introducing a new variable, persistedError
that only resets to undefined once a success state is reached or reset is called. This way there is no breaking changes with error
and it is a light weight opt-in to get this behavior.
We can do the same with persistedData
I'm not against having these flags. But here is my question:
Why is it wrong to have errors and data while the request is happening? How can that affect existing users? How could this change break existing use cases?
If we update the types though to tell the truth, we lose the narrowing of types using isError, isPending, and isSuccess
In terms of types status. Let's see. In the case in my videos (while the request is happening)
isPending
keeps the same. No breaking changeisSuccess
it's false
so no breaking change in terms of this propisError
it's true
so no breaking change here either no?Good questions, happy to share my thought processes further.
TServerActionResult
will need modification.
As of now, the type doesn't support isError
and isPending
being true at the same time.
The type for a pending status has error and data both set to undefined.
{
isPending: true
isOptimistic: false
data: undefined
isError: false
error: undefined
isSuccess: false
status: "pending"
}
However, this will not be an "honest type" since error and data can be defined while status is pending -- if we go this direction. The same is true for the error types and success types, since isPending can be true at the same time.
We can't have dishonest types. Meaning we would need to update the types to correctly reflect that if status is pending, data or error can be defined.
Also keep in mind this is a validation library I think keeping errors while running is the expected behaviour
Since big libraries like tRPC
and react-query
do not keep errors while running, it can diverge from the expected behavior for some.
The breaking change off the top of my head was if people used the status to determine the shape in a specific order.
if (isError) {
return <div>Error: {error.code}</div>
} else if (isPending) {
return <div>Loading...</div>
} else if (isSuccess) {
return <div>Success: {JSON.stringify(data)}</div>
}
This code will no longer be valid since isError and isPending can be true at the same time. People will always need to check isPending
first. I think if people are coming from react-query (which we have some) this can create friction since they are used to the following
error - If the mutation is in an error state, the error is available via the error property.
data - If the mutation is in a success state, the data is available via the data property.
This PR diverges ZSA from this pattern that I think many people are used to from the big data fetching libraries. It would change to: "if the mutation is in an error
state, the error is available via the error property. Or if its in a "pending" state, it can be available."
Hi, we don't need to rush 😄. We can discuss this calmly because I respect what you're doing here. It's late and during the week my brain is fried so excuse me if I say nonsense 🙏
I'm playing with react-query api here https://codesandbox.io/p/sandbox/react-query-mutation-forked-8jyg9h?file=%2Fsrc%2FReactQueryExample.js%3A27%2C31
Indeed they remove the error while request is flying. But even if they don't the case you put would not break no?
Also in this simple example, you can see the jumpy text. I think react-query is a general purpose tool so it can have this behavior but a tool that works for data mutation in forms looks better to keep old data if you think in terms of UX for forms to avoid jumpiness.
I agree types can NOT lie and should be changed. But still can't see the breaking code if we always keep data/errors while requesting.
I'll think harder and read more carefully what you said.
All good man, totally concur, no rush. I can ask some other zsa
users for more input here. These discussions are always good and pipe out the best DX for users.
I agree his video demonstrates an issue in UX and keeping the error state around might be useful in some scenarios. I wonder if an isError || hadError would be useful so that we can then expect the error property to be defined in the type? I'm not sure if that helps.
{
isPending: true
isOptimistic: false
data: undefined
hadError: true
isError: false
error: new Error()
isSuccess: false
status: "pending"
}
I personally didn't run into this issue because I'm using client side validation on all my forms, so the moment someone clears the top input after submitting, the error validation shows.
Appreciate the input here @webdevcody
I personally didn't run into this issue because I'm using client side validation on all my forms
Yeah I think this is why the big libraries like tRPC
and react-query
don't persist the error is because its common to have client side validation in place when using forms.
Overall I am on board with adding something like a hadError
or persistedError
to keep the TServerActionResult
roughly the same while maintaining type honesty.
Overall I am on board with adding something like a hadError or persistedError to keep the TServerActionResult roughly the same while maintaining type honesty.
Ok, let's go with this. I'll add the flags
its common to have client side validation in place when using forms
With the new input schema functions that will soon be released in the next minor version, validation might not be able to be fully done on the client (as in @andresgutgon) case where I think he is checking for duplicate usernames
Ok, let's go with this. I'll add the flags
Awesome. Can you make a PR into the input-fn
branch? I will merge that one with all these changes together.
Can you make a PR into the input-fn branch?
You mean rebase this one with input-fn
and point to that branch?
Nevermind, no need to point there. Just merged that one to main
I made all this behavior optional on zsa
and zsa-react
. What do you think @IdoPesok?
I think these are the two places I need to touch to make this change fully backward compatible.
This is how we can enable this behaviour
export const onboarding = authProcedure
- .createServerAction()
+ .createServerAction({ persistedDataWhenError: true })
const { data, executeFormAction, isPending, error } = useServerAction(onboarding, {
initialData: { name, username },
+ persistedData: true,
+ persistedError: true,
I'm happy you convinced me to add these flags. I can't handle in my head 🤯 all the use cases this package manages. it's better if it's backward compatible :)
For feedback, I am a bit confused with naming of the persistedDataWhenError
in use server action. To me, data refers to the returned data of the handler. However, this is the input the action receives. I was debating adding a .includeInputInError
method to procedures and actions that will add the raw input to TZSAError. Maybe we just always include raw input in errors?
However, this is the input the action receives. I was debating adding a .includeInputInError method to procedures and actions that will add the raw input to TZSAError.
This is too much imo. Forcing to implement a method
Maybe we just always include raw input in errors?
So no check? always return the rawInput. I like this if you think is not going to break existing use cases
True, so yeah let's just add raw input to the default TZSAError. Won't break anything. The one concern is security, which is why we can't return parsed input but raw input is fine since that's what gets passed in so guaranteed nothing leaks.
Opps! i closed by mistake
The one concern is security, which is why we can't return parsed input but raw input is fine since that's what gets passed in so guaranteed nothing leaks.
So that means zsa-react has to deal with FormData?
Why is a security concern? I want to understand
To me, data refers to the returned data of the handler. However, this is the input the action receives.
I'm thinking more about this. This is not the case no? When there is an error this input data does not arrive to the handler. Imo having this flag as part of the action configuration is clear enough. But open to change
I would like to understand your concerns with security and also not returning always JSON which force zsa-react
do that work.
Another thing I don't like about returning inputData in TZSAError
is that it force the frontend to consume the data in two different ways when there is an error when it is not
🟢 This is how I use it now:
const { data, executeFormAction, isPending, error } = useServerAction(...)
const fieldErrors = error?.fieldErrors
return (
<Input
label='Your name'
errors={fieldErrors?.name}
defaultValue={data?.name}
/>
)
This api makes super nice to build pure transactional backend-only experiences
🔴 Having inputRow
in TZSAError
would mean having to extract data for the input from two places
const { data, executeFormAction, isPending, error } = useServerAction(...)
const nameData = data?.name ?? error.rawInput?.name // Or something like this
Hi @IdoPesok could you take a look at my comments? I finished my onboarding flow! 🎉
Everything works great now but I would love to merge this PR before merging my code
Hi, sorry for the late response. Congrats on the onboarding flow, UI looks sick.
There is a bug with persistedData
I am noticing when forms have files. It will try to send the file back but this is not allowed.
Also, I cherry picked in your commit that fixes side effects with executeFormAction
. Thanks for catching that 🙏
There is a bug with persistedData I am noticing when forms have files. It will try to send the file back but this is not allowed.
Oh good catch. So a form with an input file right? I didn't tried this case. I'll try to fix it
Ok I think what we can do is to set to undefined
the File
inputs. React server actions only allow primitives values according to documentation
https://github.com/IdoPesok/zsa/assets/49499/d1b24672-1a41-405a-9663-72a40ea743d8
Happy to report the persist flags are available in zsa-react@0.2.1
🫡
Return data from action when there is an error (used in frontend to avoid ending with loss data)
Trying a bit diff implementation of this. ETA 1 day.
Happy to report the persist flags are available in zsa-react@0.2.1 🫡
You mean the changes in this PR? I see, the frontend part 👌
Trying a bit diff implementation of this. ETA 1 day.
Happy to see it. What's the idea?
I see you 🐮 👦 in main
directly. Is one of the perks of being a maintainer 😂
Happy to see it. What's the idea?
New PR: #155. The issue with the code in this PR is that it is breaking one of the core behaviors of ZSA which is to return [null, err]
or [data, null]
. We can not return this type for useServerAction
only which is where opts comes in. There are a few edge case tests that are not passing and I want to write more tests before I merge, but the implementation is done.
Awesome work! I'll close this and follow yours for progress
What?
This is useful if we use
data.my_field
in the UI. If other fields have errors butmy_field
is ok I want to keep showing it.Fix this issue https://github.com/IdoPesok/zsa/issues/121
TODO
Current behavior
https://github.com/IdoPesok/zsa/assets/49499/2b3eea1a-c410-4f0f-bd3d-8b7b7bef627b
With these changes
https://github.com/IdoPesok/zsa/assets/49499/e82058c6-22a1-4be1-ae5c-65359d1a3c06