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 removes rest object props #481

Open
vladshcherbin opened this issue Mar 14, 2024 · 12 comments
Open

Omit removes rest object props #481

vladshcherbin opened this issue Mar 14, 2024 · 12 comments
Assignees
Labels
enhancement New feature or request intended The behavior is expected priority This has priority

Comments

@vladshcherbin
Copy link

Rest object props with unknown() are removed by omit()

Example:

import { object, omit, parse, string, unknown } from 'valibot'

const data = {
  a: 'a',
  b: 'b',
  c: 'c',
  d: 'd'
}

const schema = omit(
  object({
    a: string(),
    b: string()
  }, unknown()),
  ['a']
)

console.log(parse(schema, data))

Current output

{
  b: 'b'
}

Expected output

{
  b: 'b',
  c: 'c',
  d: 'd'
}

valibot 0.30.0

@fabian-hiller
Copy link
Owner

fabian-hiller commented Mar 14, 2024

This is the intended behavior. It is documented in our API reference, but I can understand why this might be confusing. I will think about it for further development of the library.

Hint: You can add the rest argument again when calling omit.

@fabian-hiller fabian-hiller self-assigned this Mar 14, 2024
@fabian-hiller fabian-hiller added the intended The behavior is expected label Mar 14, 2024
@vladshcherbin
Copy link
Author

@fabian-hiller I've read the note after opening the issue, still didn't figure out the must be added again part to make expected output. Used transform as a workaround for now.

If it's somehow possible to make it work using must be added again (adding unknown() again or smth else) I'd be happy to see an example and add to the docs 🙌

@fabian-hiller
Copy link
Owner

Check out our playground for an example.

import * as v from 'valibot';

const ObjectSchema1 = v.object(
  {
    key1: v.string(),
    key2: v.string(),
    key3: v.string(),
  },
  v.unknown()
);

const ObjectSchema2 = v.omit(
  ObjectSchema1,
  ['key2'],
  v.unknown() // <-- Add `rest` again
);

@vladshcherbin
Copy link
Author

@fabian-hiller thank you for the example. I'm still confused 😅

Here's my playground

From what I expect, key2 should be omitted.
However it's still in the output.
Basically ObjectSchema2 does nothing 🤔

This key is also removed from autocomplete but we see it's there in the output:

image

I remember trying it with the same result.

p.s. it may be the intended behavior and just not the one I'd expect from the code

@fabian-hiller
Copy link
Owner

fabian-hiller commented Mar 15, 2024

Because of the rest argument v.unknown(), the schema allows any unknown entry to pass. This includes key2 even if you have omitted it.

@vladshcherbin
Copy link
Author

@fabian-hiller yeah, it kinda reverts what omit() does.

So basically it's not possible to omit key2 from object and leave rest unknown props untouched using omit() function, only transform can do this.

I'd consider this a bug in omit() since it clearly does more than just omitting key2 by omitting all unknown props from the object.

@fabian-hiller
Copy link
Owner

How would you do the same with pure TypeScript?

@vladshcherbin
Copy link
Author

@fabian-hiller a quick one:

const data = {
  a: 'a',
  b: 'b',
  c: 'c',
  d: 'd'
}

function omit<T, K extends keyof T>(object: T, keys: K[]): Omit<T, K> {
  const result = { ...object }

  keys.forEach((key) => {
    delete result[key]
  })

  return result
}

const result = omit(data, ['a', 'b'])

console.log(result)
console.log(result.c)

I'd probably use some kind of Object.keys, Object.entries, Object.fromEntries or reduce but it requires a lot more time spent on typings 😅

@fabian-hiller
Copy link
Owner

And how would you implement it if you wanted it to allow unknown entries to pass? I am not sure if I understand you correctly, but I think you want to obmit a specific key, but also allow any unknown key to pass. If that's your goal, there's probably only one solution: You need to explicitly declare that key as never.

@vladshcherbin
Copy link
Author

vladshcherbin commented Mar 15, 2024

Yes, thats exactly what I want - omit only specific key and leave the rest untouched (be it specified keys from schema or unknown ones).

// initial object
const data = { a: 1, b: 2, c: 3, d: 4 }

// remove ONLY a
// result is { b: 2, c: 3, d: 4 }
omit(data, ['a'])

// remove ONLY a AND c
// result is { b: 2, d: 4 }
omit(data, ['a', 'c'])

incorrect

I've tried Typescript Omit for correct output type but it's not smart enough for index signature:

type Data = {
  [key: string]: unknown
  a: number
  b: number
  c: number
  d: number
}

type Omitted = Omit<Data, 'a'>
image

The output type loses the overview of defined keys.

correct

The correct one can be seen using Except from magical 🧙 type-fest:

import type { Except } from 'type-fest'

type Data = {
  [key: string]: unknown
  a: number
  b: number
  c: number
  d: number
}

type Omitted = Except<Data, 'a'>
image

No wonder type-fest has over 160 million downloads 😅

That's basically what I expect omit to do.

I also believe this is how other libraries work (tested on lodash omit)

@vladshcherbin
Copy link
Author

vladshcherbin commented Mar 15, 2024

Same bug conclusion from type-fest with identical examples and expectations - sindresorhus/type-fest#382

Good read on the topic - microsoft/TypeScript#30825

@fabian-hiller
Copy link
Owner

Thank you very much! I will look into this in the next few weeks. Maybe we should also add an except method in addition to the omit functionality.

@fabian-hiller fabian-hiller added enhancement New feature or request priority This has priority labels Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request intended The behavior is expected priority This has priority
Projects
None yet
Development

No branches or pull requests

2 participants