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

Odd behavior: when missing a data field, uses server instead of custom validators #243

Closed
jtlapp opened this issue Aug 4, 2023 · 20 comments

Comments

@jtlapp
Copy link

jtlapp commented Aug 4, 2023

I'm developing a TypeBox adapter for Superform and encountered an odd behavior.

If I provide superForm with a validators object and my custom superValidate returns a data object that is missing a value for a property found in the form, superForm completely ignores the validators object and instead hits the server for validation.

Your superValidate provides defaults for all fields, and I can certainly do that, so this is not a problem for me.

I realize that the protocol between superForm and superValidate is an internal protocol that need not provide robust support for adapters like mine, but I thought I'd report the behavior in case you might be inclined to instead emit an error so that folks making adapters (all one of us?) aren't spending hours debugging the problem.

@ciscoheat
Copy link
Owner

Sorry, but that's how it is with open source software - you pay with your time. The client validation is complicated due to the deep object comparison and tainted checks. Do you know more precisely where the problem is occurring?

@jtlapp
Copy link
Author

jtlapp commented Aug 5, 2023

I did pay quite a bit of time discovering the problem. ;-) But I didn't try to locate the problem in the superforms source.

Anyway, I've let you know about the strange behavior. If it's not immediately apparent how to deal with it, or that anything should be done, I'll close this issue.

@jtlapp jtlapp closed this as completed Aug 5, 2023
@ciscoheat
Copy link
Owner

I appreciate you reporting it, but I'm not sure how to deal with the multitude of validation libraries, that I really would like to support with Superforms (with something like typeschema), but given their current limitations, I cannot do that without more planning. #120 is the main issue for this.

A separate JSON Schema for completing the lack of features is an alternative, but it complicates things.

@jtlapp
Copy link
Author

jtlapp commented Aug 5, 2023

Oh, I think I see now what you were responding to.

Actually, the most helpful thing you could do for me is to treat the return value of superValidate as part of the public interface and bump the major version on any breaking change to it.

I've implemented my own superValidate for TypeBox, and it needs to work with your superForm.

As for the relevance to the present issue, I had to discover that this interface requires a default value for each field in the form. Now that I know that, I'm good, but it was odd that failing to meet this requirement caused superForm to ignore the validations object and instead hit the server. It was just something odd and likely unintentional on your part.

@ciscoheat
Copy link
Owner

It's already a part of the public interface, a type called SuperValidated:

export type SuperValidated<

Its data field is the inferred schema type, so the data must always be set according to the types. Then it depends on the schema if a field has a default value or not. The rules are described here: https://superforms.rocks/default-values

If you can figure out where this silent error occurs, I can probably add an error in a future release.

@jtlapp
Copy link
Author

jtlapp commented Aug 5, 2023

Thanks. I'm already basing my output type on SuperValidated but didn't know if you considered its specifics to be internal implementation or something meriting upping major revisions on breaking changes. And yeah, I had to plug any in for the Zod schema, though I may yet base the data return type on the TypeBox schema.

I have things generally working and am now testing, though only for superValidateSync(). I was surprised that I was able to get TypeBox to work -- a testament to how well abstracted Superforms is.

@ciscoheat
Copy link
Owner

Glad you've gotten this far, it's hopeful for an eventual typeschema integration in v2.0. :) Any Superforms-related reason you've made a sync version only?

@jtlapp
Copy link
Author

jtlapp commented Aug 5, 2023

It's sync-only for now just for proof of concept. I don't want to create unnecessary machinery if the idea fundamentally won't work.

I'm basing the solution on my typebox-validators library, which offers optional lazy compilation with caching and failing at first error, among other things. Any generic solution would need to be compatible with features like these that Zod doesn't support. TypeBox itself has optional compilation, though not lazy and cached.

@ciscoheat
Copy link
Owner

I'd like to see if there's anything that can be done easily enough for this. You say that the problem occurs when data returned from superValidate doesn't correspond to the schema type (like a missing field) - in that case, client-side validation fails?

What kind of validation is then used on the client, a Zod schema or the built-in Superforms validators?

@ciscoheat ciscoheat reopened this Aug 5, 2023
@jtlapp
Copy link
Author

jtlapp commented Aug 5, 2023

When I get a chance, I'll have a look at it again and see if I can get you code that does it.

@jtlapp
Copy link
Author

jtlapp commented Aug 6, 2023

I'm starting to get a sense of what it would take to generalize superValidate to support TypeBox. I think this would be the better solution, because much of the HTTP handling code is the same, and the remaining code would be structured similarly as well. But for now, I just want to hardcode a TypeBox solution, as that'll be easier and faster, and I want to be certain it's possible before investing more effort.

@jtlapp
Copy link
Author

jtlapp commented Aug 6, 2023

Why do form actions assign defaults for missing values after submission to the server? This is an expensive process to perform, especially when the defaults should already have been provided client-side.

The main reason people choose TypeBox over Zod is that it benchmarks at about 200 times faster. I'd rather try to keep a TypeBox solution performant.

@jtlapp
Copy link
Author

jtlapp commented Aug 7, 2023

Another question for ya: Why do you hash the schema instead of just creating a random string with virtually zero chance of conflict? Seems like the latter would be much more efficient. I'm not seeing the hash used to equate different instances of identical schemas. It seems to just be a unique ID.

EDIT: I originally wrote "random number" but meant "random string."

@jtlapp
Copy link
Author

jtlapp commented Aug 7, 2023

I'm now questioning the suitability of using Superforms with TypeBox. The Zod ecosystem is rich, while there is very little for TypeBox. When people choose TypeBox, it is because they're prioritizing performance.

To prioritize performance with Superforms, I would compile and cache each schema's form-parsing code. But then even better performance could be gained by submitting JSON and relying on the built-in JSON-parsing library. And if we're submitting JSON, we have JS on the client and so little need to use SvelteKit form actions.

But I still highly value the amazing work you've done on form usability, so I'll be thinking of the best way to leverage that.

In case you're curious, I've written probably 95% of what is needed to support TypeBox with the Superforms approach. I didn't quite finish the async superValidate(), but the missing bits are bits you'd understand better than I would. The port gave me quite an education. Thank you for that!

@ciscoheat
Copy link
Owner

ciscoheat commented Aug 7, 2023

Why do form actions assign defaults for missing values after submission to the server? This is an expensive process to perform, especially when the defaults should already have been provided client-side.

To ensure type-safety for the schema data. If a schema field is defined as z.string() it cannot be undefined or null, and since you can't guarantee any posted data, a default value must be added. All the metadata is cached after first use, so it's not expensive.

The main reason people choose TypeBox over Zod is that it benchmarks at about 200 times faster. I'd rather try to keep a TypeBox solution performant.

There's a big difference between type validation and html form validation over http. Forms aren't in a hot code path, so I'd be surprised if form posting with a "slow" library (Zod isn't slow, just not fastest) will cause a performance issue. Still waiting for the first report of that. :)

Why do you hash the schema instead of just creating a random string with virtually zero chance of conflict? Seems like the latter would be much more efficient. I'm not seeing the hash used to equate different instances of identical schemas. It seems to just be a unique ID.

The hash is cached so there's no performance problem. It's a backwards compatibility issue, not setting an id counts as the same id, at least for each schema. That will probably change to a unique hash, as you say, in v2.0.

@ciscoheat
Copy link
Owner

The missing property problem that you initially reported should be fixed now with 1.5.1. All validators are now checked against missing properties, and a console error is now reported if a validator throws an exception.

@jtlapp
Copy link
Author

jtlapp commented Aug 10, 2023

Thanks for fixing that!

Sorry I've been out of touch these past few days. An amazing opportunity has come up for me, and I was trying to make a decision.

Why do form actions assign defaults for missing values after submission to the server? This is an expensive process to perform, especially when the defaults should already have been provided client-side.

To ensure type-safety for the schema data. If a schema field is defined as z.string() it cannot be undefined or null, and since you can't guarantee any posted data, a default value must be added. All the metadata is cached after first use, so it's not expensive.

Yeah, I'm changing my mind about this. Assigning defaults isn't that much more expensive because the server must iterate over the provided values anyway, to parse them. Technically, it's only necessary to assign defaults for values that or optional or can be null, since FormData can't represent these values; the server could require the client to provide all other values, including their defaults if that's what the client wants. This approach does present a problem for evolving form actions that must work with third party clients, because it prevents the server from providing default values for new fields that the client might not yet support. An unlikely problem, but still a potential one.

The main reason people choose TypeBox over Zod is that it benchmarks at about 200 times faster. I'd rather try to keep a TypeBox solution performant.

There's a big difference between type validation and html form validation over http. Forms aren't in a hot code path, so I'd be surprised if form posting with a "slow" library (Zod isn't slow, just not fastest) will cause a performance issue. Still waiting for the first report of that. :)

I think the fact that TypeBox exists and is popular explains why you won't be getting complaints about slow performance. It's likely that Zod itself is the limiting factor in your case. The folks would would express concern won't be using Zod.

Even so, it should be possible for a TypeBox user to decide that a form is not on a critical path and use TypeBox with form actions.

Why do you hash the schema instead of just creating a random string with virtually zero chance of conflict? Seems like the latter would be much more efficient. I'm not seeing the hash used to equate different instances of identical schemas. It seems to just be a unique ID.

The hash is cached so there's no performance problem. It's a backwards compatibility issue, not setting an id counts as the same id, at least for each schema. That will probably change to a unique hash, as you say, in v2.0.

It'll affect cold starts in serverless environments.

Anyway, I'm coming back to this project with a fresh perspective. I'm looking at creating a package that provides the services Superforms needs to support TypeBox, while still being independent of Superforms. I'll make sure I can integrate it properly with an other package that uses Superforms with TypeBox before committing to making it available, though. That way I can get Superforms support for TypeBox now, and you'd get a little assistance with TypeBox support in Superforms 2.0.

Thanks again for fixing the bug, and for helping me to understand Superforms better.

@jtlapp
Copy link
Author

jtlapp commented Aug 16, 2023

Here's a package you can use for TypeBox integration: typebox-form-parser.

@ciscoheat
Copy link
Owner

Thank you, though it is quite a task to make everything work, especially on client-side and with correct typing, as mammoth types like these are quite integrated with Zod. If they are possible to convert to TypeBox or any other library, I'll be quite impressed...!

@jtlapp
Copy link
Author

jtlapp commented Aug 16, 2023

Thank you, though it is quite a task to make everything work, especially on client-side and with correct typing, as mammoth types like these are quite integrated with Zod. If they are possible to convert to TypeBox or any other library, I'll be quite impressed...!

I don't know Zod, but TypeBox supports introspection of nested objects because it is valid JSON Schema. Anything you can do with JSON Schema, you can do with TypeBox.

My above library doesn't support nested objects at present, but it does support nested arrays and unions in meaningful circumstances.

Client-side TypeBox validation is easy with the custom validators object. However, I'm putting my TypeBox superforms implementation on hold for a while to deal with a more pressing issue.

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

No branches or pull requests

2 participants