fabian-hiller / valibot

The modular and type safe schema library for validating structural data 🤖
https://valibot.dev
MIT License
5.6k stars 169 forks source link

omit doesn't work with objectWithRest #678

Open vladshcherbin opened 1 week ago

vladshcherbin commented 1 week ago

omit is not working when used with objectWithRest schema

example:

const schema = v.omit(
  v.objectWithRest({
    key1: v.string(),
    key2: v.string()
  }, v.unknown()),
  ['key1']
)

const data = {
  key1: 'a',
  key2: 'b'
}

v.parse(schema, data)

expected

omit removes specified key1 entry, same as using object schema:

{
  key2: 'b'
}

actual output

omit doesn't remove any entry:

{
  key1: 'a',
  key2: 'b'
}

valibot 0.34

example playground

related - https://github.com/fabian-hiller/valibot/issues/481

fabian-hiller commented 1 week ago

From my point of view, this is the intended behavior. Valibot tries to behave similar to TypeScript and Omit only omits known properties.

We can leave this issue open for now to see if more developers see this differently and if that's the case we can rethink omit or add another method called except that returns an issue for each excepted key.

sillvva commented 1 week ago

I will say that, at the very least, it's unintuitive. However, from a TypeScript behavior view, Fabian is right. The resulting type is what you'd expect, but the data still conforms to that type.

const schema = v.omit(
  v.objectWithRest({
    key1: v.string(),
    key2: v.string()
  }, v.unknown()),
  ['key1']
);

// resulting type
type Schema = {
    key2: string;
} & {
    [key: string]: unknown;
}

// No error
const data: Schema = {
  key1: 'a',
  key2: 'b'
}

// Same: No error
const result = v.parse(schema, data);
console.log(result);

What you're looking for is to transform the result based on the properties you want to omit, rather than conforming to the normal omit behavior. This schema will do what you want.

const newSchema = v.pipe(
  v.objectWithRest({
    key1: v.string(),
    key2: v.string()
  }, v.unknown()),
  v.transform(({ key1, ...rest }) => rest)
);

Playground Example

@fabian-hiller I do think there should be a more intuitive method/action for this. I like the idea of having an except, that is like Omit, but does a similar transformation.

fabian-hiller commented 1 week ago

omit "deletes" the selected entries when initializing the schema. This results in a smaller bundle size and better performance. except could work differently and check during parsing if any of the selected keys are included. @vladshcherbin what should be the output type of such a function? Should we mark the selected keys as never or should the type be the same as with omit?

vladshcherbin commented 1 week ago

@fabian-hiller, the omit output is good since it excludes the specified keys. never seems stricter but unnecessary; either should work fine 👍

However, as @sillvva noted, the behavior is unintuitive.

For example, using omit with object works as expected, removing the specified keys. But if I switch to objectWithRest, omit suddenly stops working 😱. This unexpected change could lead to confusion and wasted time finding workarounds like transform or except. Additionally, if except works like omit but removes keys correctly for any object schema, why use omit at all? 🤷‍♂️

As you mentioned, Omit in TypeScript should omit/remove/"delete" known properties. In my example, key1 is known, yet omit has no effect. The result includes this key, which doesn’t match the expected behavior.

I strongly believe omit should work consistently across any object schema without needing workarounds.

fabian-hiller commented 1 week ago

Here is another workaround that can be used in the meantime:

const Schema = v.objectWithRest(
  { key1: v.never(), key2: v.string() },
  v.unknown()
);

In my example, key1 is known, yet omit has no effect. The result includes this key, which doesn’t match the expected behavior.

It only has no effect because the specified rest is marked as unknown. If you specify a specific data type, this is not the case. As I wrote, the current behavior is the intended behavior from my point of view. Until I get more feedback from other developers, I will not make any changes for the time being.

If we should change the behavior of omit, I would like to point out that we will then lose type safety, since unknown keys must also be able to be passed via the keys argument.

vladshcherbin commented 1 week ago

@fabian-hiller I've tried this workaround but it gives an error when the key exists. The transform one works as expected and is a valid solution until omit is fixed or is obsolete by a new superior except function.

If we should change the behavior of omit, I would like to point out that we will then lose type safety, since unknown keys must also be able to be passed via the keys argument.

can you please provide a quick example?

btw huge thank you for the library and all the time you spend on its improvements and conversations ♥️

fabian-hiller commented 1 week ago

can you please provide a quick example?

Currently, you get a TS error if you specify an invalid key: v.omit(v.object({ key1: v.string() }), ['keyX'])

vladshcherbin commented 1 week ago

@fabian-hiller that's what I've thought about, thank you.

I'm not so deep into TS but is it possible to have omit give you a TS error for an invalid key when used with object and don't give an error when used with objectWithRest?

fabian-hiller commented 1 week ago

Yes, that could be possible.