Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(servervalidationerror): adds a ServerValidationError #52

Merged
merged 5 commits into from
Feb 2, 2024

Conversation

theboxer
Copy link
Contributor

I've encountered in almost every project that interacts with an external API a need to return a validation error from the server action. Common example could be when creating a new record that has a unique key (for example email). The server action calls the API and get an error that the email already exists. I usually hacked this and returned a response from server action in similar format to:

{
  "success": false,
  "errors": {
    "email": "Email already exists"
  }
}

Then on client side, there's a need for an additional check on the returned data.

This PR aims to add a way for the user to trigger validationErrors output from the server action, instead of the described approach above, there's new function passed to the server action, that lets you trigger the valdiationErrors.

const input = z.object({
	email: z.string(),
});

export const registerUser = action(input, async ({ email }, _ctx, serverValidationError) => {
        const emailExists = await db.query.users.findFirst({ where: eq(users.email, email) });       
	if (emailExists) {
		serverValidationError({
			email: ['Email already exists'],
		});
	}
});

On the client side, this will return:

{
  "validationErrors": {
    "email": ["Email already exists"]
  }
}

Which removes the need to do an additional check on the data, but you can rely on that if validationErrors and serverErrors are null, your server action succeeded.

Side note: I initially exported the ServerValidationError and wanted the user to throw it manually, but that would require passing typeof schema or schema to the error, to be able to hint possible validation error keys. The current approach with helper method doesn't require that, but I'm not 100% sold on it....

Copy link

vercel bot commented Jan 16, 2024

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

Name Status Preview Comments Updated (UTC)
next-safe-action-example-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 1, 2024 1:55pm
next-safe-action-website ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 1, 2024 1:55pm

@TheEdoRan
Copy link
Owner

Hi @theboxer, thanks for your contribution. I think this is very useful and the optimal way to deal with custom validation errors set in server code execution. Though, I already implemented this pattern in all of my projects via the "native" way, which is using the validation library to do the job.

I closed an issue yesterday about the same problem, check out the comment I left there. I think this approach is pretty good already, but please let me know what you think about it. Thanks!

@theboxer
Copy link
Contributor Author

@TheEdoRan thanks for that example, I actually think that it make perfect sense to deal with the validation errors as natively as possible. I actually didn't think about the option to add new set of validation to the same schema server side and perform the db validation on this level. I will update my code to use this approach :)

However, not every project I run uses local data access and will be able to leverage the native schema. When I interact with the data through a remote crud API, it's impossible to do the check beforehand in the schema. I think this instance would be ideal for the proposed solution in this PR.

What do you think?

@TheEdoRan
Copy link
Owner

When I interact with the data through a remote crud API, it's impossible to do the check beforehand in the schema.

You can interact with external APIs in .superRefine() at the schema level, but the problem is that then there's no way to pass data to the action's server code function, so yeah, it's pretty limited in functionality.

I think your solution is great if the native way cannot be used. However, I also think that:

  1. We need to merge schema validation errors from the validation library and custom ones set with serverValidationError;
  2. We have to decide if it's better to put the serverValidationError function, along with other potential future util functions, in an object as the second or third argument of the action's server code function. Let me explain better:
  • if we decide that it's better to keep these util functions as the third argument of the action's server code function, the API would look like this one below. The only difference from your implementation is that these functions are in an object (see the curly braces):
export const registerUser = action(schema, async (input, ctx, { serverValidationError }) => {
  ...
});
  • if we instead decide that it's better to put these util functions in an object as the second argument of the action's server code function, the current API would break, because the second argument is currently reserved for the optional context data returned by the middleware() function, passed to createSafeActionClient() (if provided). The server code function would only have two arguments in this case, and the context would be placed in the same object as the util functions, in a property called ctx:
export const registerUser = action(schema, async (input, { serverValidationError, ctx }) => {
  ...
});

I still have to decide what the best approach is. First one could be included in version 6, since it only adds a feature and breaks nothing, second one with the breaking change would be included in version 7. Also, I think it's probably better to be a little more explicit and name this function something like setServerValidationErrors.

Please let me know what you think about this, and thank you for the patience.

@theboxer
Copy link
Contributor Author

You can interact with external APIs in .superRefine() at the schema level, but the problem is that then there's no way to pass data to the action's server code function

Even if this would be possible, I don't think it would be a good approach to do so, schema should be purely responsible for data validation and not calling API (by this I mean regular CUD endpoints, not specific endpoints for validation) and returning API's data as a side effect

  1. We need to merge schema validation errors from the validation library and custom ones set with serverValidationError

I don't think this is a good idea. It would force you to send unvalidated data to the server action and then to the API endpoint. I see the ServerValidationError as an extra validation step, that can't be performed with the schema itself and should only happen after the input passes the initial schema validation to ensure some level of integrity.

  1. We have to decide if it's better to put the serverValidationError function, along with other potential future util functions, in an object as the second or third argument of the action's server code function.

I completely agree with this one, I added it as an extra argument, because I didn't want to break the BC. I do agree that it should be an object, as there may be another utils functions later on, like you mentioned. I'd also vote for v7 to merge it with context and have it as a second argument.

For the name, maybe it could be even more explicit as throwServerValidationErrors as it actually throws an error and exists the code (to avoid a mistakes with calling that function several times and expecting it'll execute them all).

@TheEdoRan
Copy link
Owner

TheEdoRan commented Jan 25, 2024

Even if this would be possible, I don't think it would be a good approach to do so, schema should be purely responsible for data validation and not calling API (by this I mean regular CUD endpoints, not specific endpoints for validation) and returning API's data as a side effect

I mean, I don't see a huge problem with this approach, because it's just all server code in the end, but I understand your point of view, it could create some confusion. And as I said, data cannot be passed to the action's server code function, so it's pretty useless in most cases anyway.

I don't think this is a good idea. It would force you to send unvalidated data to the server action and then to the API endpoint.

Don't know why I didn't think about this. Yes, this is pretty terrible. If the server receives invalid data and we don't stop execution before reaching action's server code function, we would get invalid parsed input as the first argument of the action's server code function, so I absolutely agree with you.

I'd also vote for v7 to merge it with context and have it as a second argument.

Yes, I also think this is better.

For the name, maybe it could be even more explicit as throwServerValidationErrors as it actually throws an error and exists the code (to avoid a mistakes with calling that function several times and expecting it'll execute them all).

Hmm, yes, internally we throw a ServerValidationError, but externally we just return the validationErrors object to the user. So, the execution is stopped when the util function is called, but it's like Next.js redirect() or notFound() functions. The average user doesn't really know that an internal error is thrown to make them work. Maybe we could call it returnServerValidationErrors or just returnValidationErrors (since we already know that we're in the server scope and the errors we set with this function will populate the same validationErrors object that the schema parsing would set). Furthermore, the user shouldn't really be able to call this function several times in action's server code function, because if a function returns never (throws), the code below the function invocation is "grayed out", indicating that it can't run.

Lastly, I'm currently actively working on and thinking about the optimal way to solve #51. Once I find the right solution, I'll push the changes to the next branch, which is a pre-release channel, for the upcoming v7. I'll then update this PR base branch to next, since it introduces the breaking change we've already discussed about in previous comments.

Please let me know what you think about all of this, and thank you again for your contribution!

@theboxer
Copy link
Contributor Author

I'd also vote for v7 to merge it with context and have it as a second argument.

I think I expressed this one in a wrong way. I wanted to say I'd love to see it in v6 as a third argument as it's fully BC and then for v7 I'd love to see it merged with the context. But if you want to go with just v7 I'm fine with that and can prepare the changes. Let me know.

I agree with the rest and went ahead and renamed the function to returnValidationErrors.

Let me know if there's something else I should update or add (like different example vs adjusting the login one)

@TheEdoRan
Copy link
Owner

But if you want to go with just v7 I'm fine with that and can prepare the changes. Let me know.

Yeah, I think this is the better solution, to hopefully avoid additional user confusion.

I agree with the rest and went ahead and renamed the function to returnValidationErrors.

Great!

Let me know if there's something else I should update or add (like different example vs adjusting the login one)

I'd say keep them as they are right now, since the new validationErrors object structure is implemented in next branch, and the main focus is to adapt the implementation for the new code. I'll now update the base branch of this PR from main to next, as these changes will land in v7. We'll also need to update the documentation once this feature is implemented. Thank you!

@theboxer
Copy link
Contributor Author

Rebased the PR with next branch and merged the utils & ctx

…can throw from server action

When user throws the server validation error, it produces validationErrors in same way as when
schema validation fails.
Rather than having the throwSerrverValidationError as a solo function that's passed to the server
@TheEdoRan
Copy link
Owner

I'm working on your code and really want to implement this feature ASAP. Problem is, I don't know why but my editor (VS Code) doesn't gray out the code below returnValidationErrors. If instead I define a function that returns never in the same file and call it inside the action's server code function, it works as expected.
I even tried to define the type of returnValidationErrors inline, inside the Utils type:

type Utils<S extends Schema, Context> = {
  ctx: Context;
  returnValidationErrors: (validationErrors: ValidationErrors<S>) => never;
};

It still doesn't work, same behavior as your implementation that uses typeof. I guess this is a TypeScript problem, any ideas?

image

@TheEdoRan
Copy link
Owner

TheEdoRan commented Feb 1, 2024

UPDATE: yes, this is a TypeScript bug (?).

I set up a new project with just a .ts file and without using next-safe-action, same behavior.

image

I also found out that if you don't destructure the object, it works as expected:

image

This isn't good. Maybe we should keep the util functions as the third argument of the action's server code function and tell users to avoid destructuring the object. I'm not a fan of this, but I suppose it's the only approach we can adopt that's "good enough".

@theboxer
Copy link
Contributor Author

theboxer commented Feb 1, 2024

Ugh, I found few TS issues on GH that this is kind of expected behaviour - nothing for the destructing not working.....

We can also ditch this completely and go back to exposing the actual error class, so instead of calling utils function, you'd do:

throw new ServerValidationError<typeof schema>({
  username: {
    _errors: ['invalid']
  }
});

It's not that pretty, but works....

…s` function

A TypeScript limitation makes impossible to get correct visualization of stopped code execution when
using a function that returns `never` (throws an error) from a destructured object. Instead, using a
standalone exported function to return validation errors to the client actually works as expected.
@TheEdoRan
Copy link
Owner

I exported a standalone function named returnValidationErrors. It requires passing a schema (even if it's not actually used in the function) to get inferred type safety. Action's server code function is back to just two arguments, parsedInput and optional ctx. I think this implementation is actually pretty good, since it doesn't introduce breaking changes and still preserves type safety. The only drawback is that you have to import the function every time you want to set custom validation errors, but it's not that big of a deal, at least in my opinion. Please let me know what you think about it.

@theboxer
Copy link
Contributor Author

theboxer commented Feb 1, 2024

I'm not sure if passing an unused var is better than passing the schema type, but not a huge deal. I like it and thank you!

Also, can this go to v6 as it doesn't break BC?

@TheEdoRan
Copy link
Owner

I'm not sure if passing an unused var is better than passing the schema type

I agree that it's not a wonderful solution, but I think it's the least painful way to implement it, because you get:

  • Less noise than <typeof schema> every time you want to use it;
  • Enforced type safety. You could omit the generic type, but you can't omit the first argument of a function.

So while I understand that passing an argument that isn't actually used is not that great, I also think that in the end it provides the best DX for this use case.

Also, can this go to v6 as it doesn't break BC?

It could go in v6, but I prefer to keep this in the next release channel, since the returnValidationErrors() function uses ValidationErrors type to set errors with the new format (the one that emulates Zod's format() function). If we land this feature in v6, then users would also need to update all the returnValidationErrors() calls inside actions when version 7 will be released.

Once this PR is merged, though, you can install the lib with npm i next-safe-action@next to get this new feature, and future migration to stable version 7 will be as simple as reinstalling it using the main release channel.

@theboxer
Copy link
Contributor Author

theboxer commented Feb 1, 2024

I agree that it's not a wonderful solution, but I think it's the least painful way to implement it, because you get:

  • Less noise than every time you want to use it;
  • Enforced type safety. You could omit the generic type, but you can't omit the first argument of a function.

So while I understand that passing an argument that isn't actually used is not that great, I also think that in the end it provides the best DX for this use case.

Fair point, I agree with that.

It could go in v6, but ...

Make sense, thanks :)

@TheEdoRan TheEdoRan merged commit 64fb643 into TheEdoRan:next Feb 2, 2024
5 checks passed
Copy link

github-actions bot commented Feb 2, 2024

🎉 This PR is included in version 7.0.0-next.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link

🎉 This PR is included in version 7.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@theboxer theboxer deleted the server-validation-error branch June 13, 2024 14:53
@theboxer
Copy link
Contributor Author

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants