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

Nested variants #794

Closed
ZerNico opened this issue Aug 22, 2024 · 14 comments · Fixed by #809
Closed

Nested variants #794

ZerNico opened this issue Aug 22, 2024 · 14 comments · Fixed by #809
Assignees
Labels
enhancement New feature or request priority This has priority

Comments

@ZerNico
Copy link
Contributor

ZerNico commented Aug 22, 2024

Hey @fabian-hiller,
i am currently trying to implement a relatively complex form using Valibot for validation.
Here is a simplified example of what I am trying to accomplish

import * as v from 'valibot';

const Schema = v.variant('type', [
  v.variant('subType', [
    v.object({
      type: v.literal('yes'),
      subType: v.literal('yes'),
      test1: v.string(),
    }),
    v.object({
      type: v.literal('yes'),
      subType: v.literal('no'),
      test2: v.string(),
    }),
  ]),

  v.variant('subType', [
    v.object({
      type: v.literal('no'),
      subType: v.literal('yes'),
      test3: v.string(),
    }),
    v.object({
      type: v.literal('no'),
      subType: v.literal('no'),
      test4: v.string(),
    }),
  ]),
]);

const result = v.safeParse(Schema, {
  type: 'no',
  subType: 'yes',
});

if (result.issues) {
  console.log(v.flatten(result.issues));
}

This works fine if the data is valid but if it is not it always goes into the first subType variant and just shows the issues of that.
Using union also is not really an option because that would give me issues for all different paths but I only want the ones for the current selection.

Something like

v.multiVariant(['type', 'subType'], [
  //...
]),

would probably work but that seems relatively hard to implement.

Is there currently a way to accomplish something like this?

Thanks in advance! :)

@fabian-hiller
Copy link
Owner

Thank you for reporting this issue. I think it is solvable, but I need more time to look into it. The main problem is that we currently ignore the discriminator for nested variant schemas. If the nested variant schemas have the same discriminator this doesn't matter, but in your case the type key is basically completely ignored.

To fix this, we need some logic that recursively checks or filters the options of nested variant schemas so that a nested variant schema is only executed if it is valid according to the outer discriminator. I know this sounds a bit complicated.

@fabian-hiller fabian-hiller self-assigned this Aug 22, 2024
@fabian-hiller fabian-hiller added enhancement New feature or request priority This has priority labels Aug 22, 2024
@fabian-hiller
Copy link
Owner

I will investigate whether it makes more sense to support multiple discriminators, or whether the outer variant schema should check its discriminator in the inner variant schemas, as written in my previous comment. I think the latter leads to more flexibility and should be preferred. I will try to extend the variant schema to support such cases with the next release.

@ZerNico
Copy link
Contributor Author

ZerNico commented Aug 23, 2024

The only advantage for multiple discriminators I see is that it makes the code a bit less messy. Depending on the situation it makes the schema less nested.
Apart from that the second variant is probably the nicer solution because it allows a lot more flexibility. I think it also matches better to the general way Valibot works.

@fabian-hiller
Copy link
Owner

I think I could solve the problem, but I need to prettify the code and write tests that cover every possible success and failure case.

@fabian-hiller
Copy link
Owner

fabian-hiller commented Aug 23, 2024

What default error message would you expect in this case? The problem is that both subdiscriminators are invalid, but if we were to return an issue for both, they would contradict each other and confuse the user.

import * as v from 'valibot';

const Schema = v.variant('type', [
  v.variant('subType1', [
    v.object({
      type: v.literal('A'),
      subType1: v.literal('A'),
      test1: v.string(),
    }),
    v.object({
      type: v.literal('B'),
      subType1: v.literal('B'),
      test2: v.string(),
    }),
  ]),
  v.variant('subType2', [
    v.object({
      type: v.literal('A'),
      subType2: v.literal('A'),
      test3: v.string(),
    }),
    v.object({
      type: v.literal('B'),
      subType2: v.literal('B'),
      test4: v.string(),
    }),
  ]),
]);

const result = v.safeParse(Schema, {
  type: 'A',
});

@fabian-hiller
Copy link
Owner

fabian-hiller commented Aug 23, 2024

I think we basically have two options. We could return only the issue for the first subdiscriminator subType1 or we could return a general issue with a subissue for each invalid subdiscriminator. The first option is easier to implement for us and our users, but the second option is "more" correct.

@ZerNico
Copy link
Contributor Author

ZerNico commented Aug 24, 2024

I'd say the second option would be the nicer one.
I think it's also closer to how variants with a single discriminator work atm?
The first one would probably lead to confusion.
Also e.g. in a form the design of the form should usually make sure that always at least one variant is hit.

@fabian-hiller
Copy link
Owner

The second option would mean that we return one general issue with two subissues, one for each missing/invalid discriminator. This is the same approach we currently use with union when we can't uniquely assign the input to one of the union options.

The "problem" with this approach is that users usually do not write code to handle the subissues. Also, generating a useful general issue for two missing/invalid discriminators is complicated. For union, we could just say Invalid type: Expected (string | number) but received true, but with variant we could have two or more missing/invalid discriminators with different received values. Therefore, it is hard to define an appropriate expected and received property for the general issue.

For now, I think we should just return one issue for the first missing/invalid discriminator. This is much easier to implement, probably more useful for the user, and it probably doesn't matter too much since the user should avoid such cases in the first place. For my provided schema, this would return the error message Invalid type: Expected ("A" | "B") but received undefined for the first subdiscriminator subType1.

@ZerNico
Copy link
Contributor Author

ZerNico commented Aug 26, 2024

I think that seems reasonable for now just showing the first invalid discriminator. For my use case that would work out fine.

@nabn
Copy link

nabn commented Aug 27, 2024

I believe i bumped into a related issue, so noting here fwiw

import * as v from 'valibot';

const LayoutConfig = v.variant('layout', [
  v.object({ layout: v.literal('text-only') }),
  v.object({
    layout: v.picklist(['hero-image', 'side-image']),
    image: v.pipe(v.string(), v.url()),
  }),
  v.object({
    layout: v.literal('carousel'),
    itemType: v.picklist(['deals']),
    deals: v.pipe(
      v.string(),
      v.transform((deals) => deals.split(',')),
      v.array(v.string()),
    ),
  }),
]);

const SplashScreenConfig = v.object({
  screen: v.literal('splashScreen'),
  backgroundColor: v.optional(v.string()),
});

const FeedConfig = v.object({
  screen: v.literal('feed'),
  position: v.pipe(v.union([v.string(), v.number()]), v.transform(Number)),
});

const DealsConfig = v.intersect([
  v.object({
    screen: v.literal('deals'),
    position: v.pipe(v.union([v.string(), v.number()]), v.transform(Number)),
  }),
  LayoutConfig,
]);

const CmsItem = v.intersect([
  v.object({
    id: v.string(),
    title: v.string(),
    url: v.optional(v.pipe(v.string(), v.url())),
    cardDescription: v.string(),
  }),
  v.variant('screen', [SplashScreenConfig, FeedConfig, DealsConfig]),
]);

v.parse(CmsItem, {
  id: '123',
  title: 'Welcome',
  screen: 'deals',
  url: 'https://google.com',
  cardDescription: 'Sample card description',
  layout: 'carousel',
  itemType: 'deals',
  deals: 'abc,def,ghi',
});

and getting

Type 'IntersectSchema<[ObjectSchema<{ readonly screen: LiteralSchema<"deals", undefined>; readonly position: SchemaWithPipe<[UnionSchema<[StringSchema<undefined>, NumberSchema<undefined>], undefined>, TransformAction<...>]>; }, undefined>, VariantSchema<...>], undefined>' is not assignable to type 'VariantOption<"screen">'.
  Property 'key' is missing in type 'IntersectSchema<[ObjectSchema<{ readonly screen: LiteralSchema<"deals", undefined>; readonly position: SchemaWithPipe<[UnionSchema<[StringSchema<undefined>, NumberSchema<undefined>], undefined>, TransformAction<...>]>; }, undefined>, VariantSchema<...>], undefined>' but required in type 'VariantOptionSchema<"screen">'.(2322)

@fabian-hiller
Copy link
Owner

No, your problem is not related to this issue. intersect is not currently supported as an option of variant. You can try to use union instead of variant:

const CmsItem = v.intersect([
  v.object({
    id: v.string(),
    title: v.string(),
    url: v.optional(v.pipe(v.string(), v.url())),
    cardDescription: v.string(),
  }),
- v.variant('screen', [SplashScreenConfig, FeedConfig, DealsConfig]),
+ v.union([SplashScreenConfig, FeedConfig, DealsConfig]),
]);

@fabian-hiller
Copy link
Owner

@ZerNico I am pretty close to the final implementation. I will explain you the new algorithm of variant probably tomorrow, so you can give me feedback before I release this change.

@fabian-hiller
Copy link
Owner

@ZerNico I described how the new algorithm works in #809. Next I will investigate if I want to support multiple discriminator keys in an array as the first argument of variant.

@fabian-hiller
Copy link
Owner

v0.41.0 is available

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority This has priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants