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

[Bug] Merge/Transform make all properties in object to optional field #40

Closed
harrytran998 opened this issue Aug 1, 2023 · 12 comments
Closed
Assignees
Labels
documentation Improvements or additions to documentation intended The behavior is expected

Comments

@harrytran998
Copy link

Hi @fabian-hiller, I have an object like this

    object({
      description: optional(string()),
      isActive: optional(useDefault(boolean(), true)),
      link: string(),
      title: string(),
      type: nativeEnum(MenuType),
    })

But when I wrap this object with transform or merge --> All properties mark optional, might be this is an expected behavior or a bug 🤔. Hope you can check this!

@fabian-hiller fabian-hiller self-assigned this Aug 1, 2023
@fabian-hiller fabian-hiller added invalid This doesn't seem right question Further information is requested labels Aug 1, 2023
@fabian-hiller
Copy link
Owner

I have checked this and for me only the properties description and isActive are optional. Can you send me your whole schema code?

@harrytran998
Copy link
Author

harrytran998 commented Aug 2, 2023

Sure, I combine two models together

export const baseSchema = object({
  createdDate: optional(string()),
  updatedDate: optional(string()),
});
export enum MenuLinkType {
  INTERNAL = "INTERNAL",
  EXTERNAL = "EXTERNAL",
}
export enum MenuType {
  NORMAL = "NORMAL",
  PROMO = "PROMO",
  NEWS = "NEWS",
}
export enum MenuLocation {
  HEADER = "HEADER",
  FOOTER = "FOOTER",
  SIDEBAR = "SIDEBAR",
}

export const createMenuSchema = merge([
  transform(
    object({
      description: optional(string()),
      endIcon: optional(string()),
      isActive: optional(useDefault(boolean(), true)),
      link: string(),
      linkType: nativeEnum(MenuLinkType),
      location: nativeEnum(MenuLocation),
      order: number(),
      parentId: optional(string()),
      startIcon: optional(string()),
      title: string(),
      type: nativeEnum(MenuType),
    }),
    (menu) => ({ ...menu, slug: slugify(menu.title) }),
  ),
  baseSchema,
]);

export type MenuModel = Output<typeof createMenuSchema>;
  1. All properties will mark as optional
  2. The new model after transform does not contain the slug field 🤔

Hope you can check this.

@fabian-hiller fabian-hiller added documentation Improvements or additions to documentation intended The behavior is expected and removed invalid This doesn't seem right question Further information is requested labels Aug 2, 2023
@fabian-hiller
Copy link
Owner

fabian-hiller commented Aug 2, 2023

Thank you for the details. This is an expected behavior. Please read through issue #26 and let me know what you think.

The important info is in these comments:

@harrytran998
Copy link
Author

Thank you for your quick response, I am not good in technical but can contribute to you the user experience from my point of view.

  1. I don't know how to execute the pipe of function in order with valibot. Might be when you execute inside to outside, the result may be correct like the expected result: Remain the object type --> transform with slug --> merge with another schema.
  2. I might think this stuff is really hard technically, but merge and transform it into basic things which anyone using zod did. I guess Zod has a trade-off to do the complicated kinds of stuff like this about bundle size or performances.
  3. If you think this was expected behavior, I think you should talk with more people using this and Zod on their production, might you can find the solutions 😆.

@fabian-hiller
Copy link
Owner

Thank you for your feedback. As far as I know, Zod does not allow to merge a transformed object and this can be technically justified. With transform, the object can be transformed to any other data type. For example a JSON string. If Valibot or Zod have no guarantee about the outgoing data, they must either discard the transformation like Valibot or prevent merging completely like Zod.

Furthermore, when merging, Valibot allows overwriting data types if the same key occurs in multiple objects. This is another reason why transform cannot be executed in this case, since otherwise incorrect data in transform or the pipeline is expected.

To make it short. If merge or another object method is applied, transform, as well as the pipeline must be added to the merged schema all over again. I will try to document this better in the long run.

@harrytran998
Copy link
Author

harrytran998 commented Aug 3, 2023

Hmm, can you suggest to me how to solve this problem with Valibot?

Bcs after using Valibot, I use this with Zod --> It worked as I expected. The code below for you can try 😆.

export const createMenuSchema = z
  .object({
    description: z.string().optional(),
    endIcon: z.string().optional(),
    isActive: z.boolean().optional().default(true),
    link: z.string(),
    linkType: z.nativeEnum(MenuLinkType),
    location: z.nativeEnum(MenuLocation),
    order: z.number(),
    parentId: z.string().optional(),
    startIcon: z.string().optional(),
    title: z.string(),
    type: z.nativeEnum(MenuType),
  })
  .transform((menu) => ({ ...menu, slug: slugify(menu.title) }))
  .and(baseSchema);

export type MenuModel = z.infer<typeof createMenuSchema>;

@fabian-hiller
Copy link
Owner

fabian-hiller commented Aug 3, 2023

Simply apply the transformation to the merged object scheme.

const createMenuSchema = transform(
  merge([
    object({
      description: optional(string()),
      endIcon: optional(string()),
      isActive: optional(useDefault(boolean(), true)),
      link: string(),
      linkType: nativeEnum(MenuLinkType),
      location: nativeEnum(MenuLocation),
      order: number(),
      parentId: optional(string()),
      startIcon: optional(string()),
      title: string(),
      type: nativeEnum(MenuType),
    }),
    baseSchema,
  ]),
  (menu) => ({ ...menu, slug: slugify(menu.title) })
);

@harrytran998
Copy link
Author

export const createMenuSchema = transform(
  merge([
    object({
      link: string(),
      title: string(),
      order: number(),
      linkType: nativeEnum(MenuLinkType),
      location: nativeEnum(MenuLocation),
      type: nativeEnum(MenuType),
      description: optional(string()),
      endIcon: optional(string()),
      isActive: optional(useDefault(boolean(), true)),
      parentId: optional(string()),
      startIcon: optional(string()),
    }),
    baseSchema,
  ]),
  (menu) => ({ ...menu, slug: slugify(menu.title) }),
);

export type MenuModel = Output<typeof createMenuSchema>;

image

export const tenantSchema = object({
  id: number(),
  name: string([minLength(1)]),
  description: string(),
});

export type TenantModel = Output<typeof tenantSchema>;

image

Hmm, really weird! Now anything turns to optional(valibot v 0.8.0)

@fabian-hiller
Copy link
Owner

For me your code works without problems and the types are displayed correctly. Does strict in the tsconfig.json have the value true?

@harrytran998
Copy link
Author

Ohhh - It worked! Thank you for your quick response! This should be in the document 😆.

@fabian-hiller
Copy link
Owner

Yes, I will improve the docs here. Thank you for your feedback!

@fabian-hiller
Copy link
Owner

Added the info to the docs. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation intended The behavior is expected
Projects
None yet
Development

No branches or pull requests

2 participants