Always return old data even when action has errors

[andresgutgon]

What?

This is useful if we use data.my_field in the UI. If other fields have errors but my_field is ok I want to keep showing it.

Fix this issue
#121

TODO

• Keep data while executing the action
• Keep errors while executing the action
• Return data from action when there is an error (used in frontend to avoid ending with loss data)

Current behavior

Screen.Recording.2024-06-16.at.16.55.18.mov

With these changes

Screen.Recording.2024-06-16.at.16.54.10.mov

[changeset-bot]

🦋 Changeset detected

Latest commit: 4566651

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
zsa-react	Patch
zsa	Patch

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

[vercel]

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

[IdoPesok]

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.

[andresgutgon]

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

[IdoPesok]

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

[andresgutgon]

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?

[andresgutgon]

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)

1. isPending keeps the same. No breaking change
2. isSuccess it's false so no breaking change in terms of this prop
3. isError it's true so no breaking change here either no?

[IdoPesok]

Good questions, happy to share my thought processes further.

Types

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.

Breaking Changes

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."

[andresgutgon]

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.

[IdoPesok]

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.

[webdevcody]

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.

[IdoPesok]

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.

[andresgutgon]

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

[IdoPesok]

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

[IdoPesok]

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.

[andresgutgon]

Can you make a PR into the input-fn branch?

You mean rebase this one with input-fn and point to that branch?

[IdoPesok]

Nevermind, no need to point there. Just merged that one to main

[andresgutgon]

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.

zsa

zsa-react

[andresgutgon]

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,

[andresgutgon]

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

[IdoPesok]

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.

[andresgutgon]

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

[andresgutgon]

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.

[andresgutgon]

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

[andresgutgon]

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

[IdoPesok]

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.

[IdoPesok]

Also, I cherry picked in your commit that fixes side effects with executeFormAction. Thanks for catching that 🙏

[andresgutgon]

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

[andresgutgon]

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

Screen.Recording.2024-06-29.at.10.49.47.mov

[IdoPesok]

Happy to report the persist flags are available in [email protected] 🫡

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.

[andresgutgon]

Happy to report the persist flags are available in [email protected] 🫡

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?

[andresgutgon]

I see you 🐮 👦 in main directly. Is one of the perks of being a maintainer 😂

[IdoPesok]

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.

[andresgutgon]

Awesome work! I'll close this and follow yours for progress