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

omit doesn't work with objectWithRest #678

Open
vladshcherbin opened this issue Jun 26, 2024 · 9 comments
Open

omit doesn't work with objectWithRest #678

vladshcherbin opened this issue Jun 26, 2024 · 9 comments
Assignees
Labels
intended The behavior is expected

Comments

@vladshcherbin
Copy link
Contributor

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 - #481

@fabian-hiller fabian-hiller self-assigned this Jun 27, 2024
@fabian-hiller fabian-hiller added the intended The behavior is expected label Jun 27, 2024
@fabian-hiller
Copy link
Owner

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
Copy link
Contributor

sillvva commented Jun 27, 2024

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
Copy link
Owner

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
Copy link
Contributor Author

@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
Copy link
Owner

fabian-hiller commented Jun 27, 2024

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
Copy link
Contributor Author

vladshcherbin commented Jun 28, 2024

@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
Copy link
Owner

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
Copy link
Contributor Author

@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
Copy link
Owner

Yes, that could be possible.

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

No branches or pull requests

3 participants