ciscoheat / sveltekit-superforms

Making SvelteKit forms a pleasure to use!
https://superforms.rocks
MIT License
2.23k stars 66 forks source link

Support for conditional validation of fields? #282

Closed phobetron closed 12 months ago

phobetron commented 1 year ago

Is your feature request related to a problem? Please describe. There does not seem to be a clear way to manage conditional fields using Superforms. By conditional fields, I mean fields that are either displayed or not, and valid or not, depending on the value of another field.

This is usually managed in other libraries that rely on zod by utilizing discriminatedUnion, but this has been mentioned in at least two previous issues (here and here) that were promptly closed.

It seems work on discriminatedUnion support has been deferred to after zod changes to switch, but there has been no movement (as far as I can see) on switch since it was mentioned nine months ago.

Describe the solution you'd like Any suggestions would be welcome, the least complex the better. My colleagues are starting to resent our technology choices 😓

Describe alternatives you've considered Currently, we are having to work around this by using refine to handle conditional validations, but this is not only a bit convoluted, but we additionally have to add our own validation triggering logic because refine doesn't seem to trigger until the rest of the schema is valid.

We could try using Felte instead, but I'd rather not use that, for some other issues we've had with that library.

ciscoheat commented 1 year ago

Discriminated unions don't unify well with forms, and especially not with Superforms, as mentioned in the issues you link to. Since the schema data maps to a single form, what you say with a discriminated union is that you basically want two different forms, and handling that in the same superForm instance is very hard for the typing system, since you cannot know the runtime value of the key that should be checked, and therefore it's not possible to expand the schema types, like $errors. The unions have to be merged and checked conditionally anyway, just in some other part of the code.

That said, I don't think it's impossible to find a solution. If you can make a simple example of what you are trying to achieve, I can take a look. Use this Stackblitz repo as a template.

ciscoheat commented 1 year ago

By the way, does Felte handle this problem? In what way?

phobetron commented 1 year ago

@ciscoheat I think the difference between other libraries, such as Felte, and Superforms is that the others are fairly well decoupled from the often multiple validation libraries they support. Usually, form fields are registered as they are attached to form elements, through various mechanisms, and not by attempting to parse a provided schema for this purpose. In particular, Felte is based on the DOM, as basic HTML forms are aware of their constituent named inputs. Felte's dynamic forms document gives some clues to their implementation.

However, Superforms' implementation at the most basic level seems to be tied directly to the parsing of a zod schema, and so it follows that basic functionality (such as mentioned in this ticket) can become as complicated to implement as zod itself, if not more. I can not comment whether the benefits of this approach outweigh its complexity, but I can say that I would gladly trade some extra type-safety or intellisense for actually being able to simply implement common UX patterns.

I'm not quite sure the utility of creating a code example for this issue, as it is extremely basic, and I'm not sure how much you would want someone to attempt a Superforms-based solution within such an example. If it were simple to mock up an example in Superforms, I wouldn't have created this issue. I'll write out a basic situation, which should be reasonable for this context.

Imagine this form:

This kind of switch is incredibly common, and this example is actually less basic than the one in that Felte document I had linked.

If Superforms requires multiple forms (meaning extra boilerplate, logic duplication, etc) to manage this, and does not intend to support this in a more straight-forward and maintainable way in the near future, it would be good to know, so that we can make more informed decisions going forward. Please don't take this as criticism; I highly appreciate the work you and others have done here, and how responsive you are to the needs of your users. I have enjoyed working with Superforms so far, so my hope is that my colleagues and I can continue to use it as we begin to implement more dynamic forms.

ciscoheat commented 1 year ago

This is what I can come up with: https://stackblitz.com/edit/sveltekit-superforms-1-testing-abbsnr?file=src%2Froutes%2Fschema.ts,src%2Froutes%2F%2Bpage.svelte,src%2Froutes%2F%2Bpage.server.ts

The key is that you have different schemas for the two entities, and validate them separately depending on the data.

There are many ways of structuring the schemas, I'm making all common fields optional in the base schema and requiring them as needed in the subclassed ones.

With that, it's as simple as using {#if $form.entity == ...} on the frontend to display the appropriate fields. As a bonus, there's a reactive statement that switches between the schemas as validators, so you won't lose the client-side validation.

ciscoheat commented 1 year ago

Also note that the baseSchema (with the optional fields) are used in the load function, otherwise the constraints and eventual default values won't be included.

phobetron commented 1 year ago

Thank you for this! This is a bit cleaner than our current implementation. I wasn't aware we could swap schemas using options.validators, which is very helpful. Maybe this solution could be documented in the Superforms docs also, until there's a more permanent solution?

Our remaining issue is that we also have an API outside of SvelteKit that offers zod validations, which we would likely interact with in SPA-mode. Those validations use things like discriminatedUnion, so we can't leverage those schemas. Additionally, early next year, we'll implement a feature which will require much more complicated branching in the schema, with possibly dozens of permutations. That could become quite unwieldy, but maybe we can get creative with this design and make it manageable.

Do you have any knowledge of when switch may be implemented in zod? I'm not on their Discord, so I don't know if there's been any recent discussion there that isn't in the GH issue.

ciscoheat commented 1 year ago

Glad you found it useful! A Zod schema is fully composable, it's one of the few libraries that does that, so you could make a function that introspects a schema and extract the union sub-schemas. This is how extract and cache schema metadata, for example: https://github.com/ciscoheat/sveltekit-superforms/blob/main/src/lib/schemaEntity.ts#L87

That whole file should be of help if you want to get creative. :) And maybe this one as well if you want it to be type-safe, though it's a lot of work to get everything right.

I understand that it's difficult if you rely on discriminatedUnion, and the more I look at it, I can't see any way of integrating it into Superforms. Not only because it's deprecated in Zod, but it's just not compatible with the "one form, one schema" idea, that makes Superforms easy to use, most of the time. I don't know if switch would work either, as it's a similar concept - splitting the type into a number of sub-schemas, that can in turn be splitted, etc. I think that's where the schema validation and form validation becomes very different in respect to each other. You can unify many schemas with a schema library, but they need to be splitted again in a form library.

I don't know about any progress for switch in Zod, sorry.

phobetron commented 1 year ago

It's really unfortunate if Superforms must treat schemas with unions/discriminated unions/switch as multiple schemas, and as such, multiple forms, as a rule that will last throughout the future of the project. Just because we can hack around with things and "make it work" doesn't mean the solution is as efficient and maintainable as what we can accomplish with other libraries. We will likely need to migrate to Felte for more complex use cases.

ciscoheat commented 1 year ago

I'm still curious how Felte would handle that, without the types being dynamic in the end.

phobetron commented 1 year ago

The complete zod integration for Felte is in this file. Since it's basically only using zod for validation, it does not need to care about zod for other things, like registering fields in the form or errors object, etc. They hook into the browser's Form API for that, IIRC (that also may be a bit of an over-simplification). This is also why they're able to support multiple schema/validator libraries.

ciscoheat commented 1 year ago

Yes, that's merely error mapping, but then you have to handle everything else regarding schema metadata (default values, constraints, data coercion) elsewhere, both on client and server.

I'm gonna make some experiments with discriminatedUnion, but I'm not optimistic. If the Zod creator even says that it was a mistake adding it, I'm hesitant to spend time on it.

phobetron commented 1 year ago

One of the down-sides with Felte is that you're on your own for transforming values/type coercion, and then you may lose some type safety when you transform values. Initial values are otherwise typed using a generic type variable, which could be inferred from the zod. As far as error reporting goes, they have several reporting strategies, one of them being an adapter to interface with the built-in Constraints Validation API. But I believe that's only for reporting, I don't think they actually use built-in browser validations internally when you provide a schema.

ciscoheat commented 12 months ago

So I've been experimenting, and the conclusion is that it's not possible to reconcile the error mapping of Superforms with Zod's discriminated unions. If you look at this schema data, which is how a discriminated union is mapped:

{
    name: string;
    entity: { type: "person", DOB: Date } | { type: "corporate", taxId: string }
}

With the Superforms error mapping, the "leaves" of this data tree is replaced with string[] | undefined:

{
    name: string[] | undefined;
    entity: { 
        type: string[] | undefined, 
        DOB: string[] | undefined 
    } | { 
        type: string[] | undefined, 
        taxId: string[] | undefined 
    }
}

See the problem here? The discriminator, the type field, isn't a discriminator anymore. So maybe the string constant could be kept there? But since it's also a normal field, that could have an error, it's not possible.

With $form it works almost as things are right now, except that the default values must be created based on merging the union objects, otherwise taxId wouldn't be initialized as an empty string, breaking type-safety.

I wish this was easily solvable, but alas, it's not. Mapping data and errors like this based on complicated branches of unions, it's a challenge.

hernandp-nimble commented 10 months ago

This is what I can come up with: https://stackblitz.com/edit/sveltekit-superforms-1-testing-abbsnr?file=src%2Froutes%2Fschema.ts,src%2Froutes%2F%2Bpage.svelte,src%2Froutes%2F%2Bpage.server.ts

The key is that you have different schemas for the two entities, and validate them separately depending on the data.

There are many ways of structuring the schemas, I'm making all common fields optional in the base schema and requiring them as needed in the subclassed ones.

With that, it's as simple as using {#if $form.entity == ...} on the frontend to display the appropriate fields. As a bonus, there's a reactive statement that switches between the schemas as validators, so you won't lose the client-side validation.

I'm having problems with this solution, The form returned on the load function is not the same type of the new validators it is throwing typescript errors whenever trying to change the options.validators property since it already has the baseSchema type

ciscoheat commented 10 months ago

Superforms v2 will handle this properly.