valibot icon indicating copy to clipboard operation
valibot copied to clipboard

Omit removes rest object props

Open vladshcherbin opened this issue 11 months ago • 12 comments

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

vladshcherbin avatar Mar 14 '24 10:03 vladshcherbin

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 avatar Mar 14 '24 15:03 fabian-hiller

@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 🙌

vladshcherbin avatar Mar 14 '24 16:03 vladshcherbin

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
);

fabian-hiller avatar Mar 14 '24 17:03 fabian-hiller

@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

vladshcherbin avatar Mar 14 '24 17:03 vladshcherbin

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

fabian-hiller avatar Mar 15 '24 04:03 fabian-hiller

@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.

vladshcherbin avatar Mar 15 '24 11:03 vladshcherbin

How would you do the same with pure TypeScript?

fabian-hiller avatar Mar 15 '24 13:03 fabian-hiller

@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 😅

vladshcherbin avatar Mar 15 '24 18:03 vladshcherbin

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.

fabian-hiller avatar Mar 15 '24 20:03 fabian-hiller

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)

  • TS issue: https://github.com/microsoft/TypeScript/issues/30825#issuecomment-523668235
  • Zod issue with suggested workaround: https://github.com/colinhacks/zod/issues/2052 (hello transform)

vladshcherbin avatar Mar 15 '24 23:03 vladshcherbin

Same bug conclusion from type-fest with identical examples and expectations - https://github.com/sindresorhus/type-fest/issues/382

Good read on the topic - https://github.com/microsoft/TypeScript/issues/30825

vladshcherbin avatar Mar 15 '24 23:03 vladshcherbin

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 avatar Mar 17 '24 15:03 fabian-hiller

Hey folks! I have been trying out the latest (0.31.0-rc.6) and was noticing some other breaking changes regarding .omit with the new pipeline changes. Can create as a separate issue if need be, but I was able to repro in the playground. At the same time, I was able to find a workaround. Maybe helpful in the docs?

edit: added playground link + wording https://valibot.dev/playground/?code=JYWwDg9gTgLgBAKjgQwM5wG5wGZQiOAIg2QBtgAjCGQgbgCh6BjCAO1XgGUmALAUxDI4AXkwA6MMDB8AFPTjiIFAFZ8mMGQG95CuAOTBSALnGTpMjGI5RgrAOYyAlABpx+w05c6FYNKgDu0AAmJpZmspbWtg4u4iC2ADJ89jA8MgAcjl4KAL7Z4hRQyKxBMoQAEnykpBCEXo4MzGwccADqwKkQAK7wopb4HTLc-IKuANqEvqgBwYQAug2MLOzwUHyoXaQwAIwi4qjI2HwACshQqLLtnT2u2gruxkTKxXwAAnwAHsjgpHxiLCBCM4dFMZlAQkRtgAmADMABYAKwANgA7OkgfQ8o1li1WtAANZnbolPZhKSyHTDfTAhSWGBFdjYaAgGQyJSOEQAPjgd10v3gmjgoMC4NcYnFAxgMD4QTgOT2SgYujgaxgXSgrDgkulQSVcvq2Oaq3WmxgUNJVkOJzOFxkeKghLwXRKtx0DxMhGerDen2+YF+-3wGJ8fhFEMI0PhyLRGKxS2aEADNQcgrWGy221cadN5rjQA

ct-gdf avatar May 29 '24 20:05 ct-gdf

This is documented here: https://valibot.dev/api/omit/

Because omit changes the data type of the input and output, it is not allowed to pass a schema that has been modified by the pipe method, as this may cause runtime errors. Please use the pipe method after you have modified the schema with omit.

I do not recommend your workaround, instead write your schema this way:

import * as v from "valibot";

const Schema = v.object({
  email: v.pipe(v.string(), v.email()),
  password: v.pipe(v.string(), v.minLength(8)),
});

const Without = v.pipe(v.omit(Schema, ["password"]), v.brand("Hello"));

fabian-hiller avatar May 29 '24 21:05 fabian-hiller

Thank you for the reply! I missed that callout in the docs. A code example would make this more apparent I think.

fwiw, my real use case had a base Schema that included a brand A that I was omitting a single property from to define the new Schema. Are you saying that the more correct approach would be to let the base Schema be defined unbranded, then derive the two variants with their own distinct brand?

ct-gdf avatar May 29 '24 21:05 ct-gdf

Are you saying that the more correct approach would be to let the base Schema be defined unbranded, then derive the two variants with their own distinct brand?

Yes, this is the recommended way at this time. I understand that this can be cumbersome in some cases. That's why I will consider adding something like a removePipe method. That way you could simply remove the pipe before applying omit. Here is an example:

import * as v from "valibot";

const Schema = v.pipe(
  v.object({
    email: v.pipe(v.string(), v.email()),
    password: v.pipe(v.string(), v.minLength(8)),
  }),
  v.brand("Hello"),
);

const Without = v.omit(v.removePipe(Schema), ["password"]);

What do you think of this idea? What could be alternative names for this function? I prefer short but still meaningful names.

fabian-hiller avatar May 30 '24 11:05 fabian-hiller

I do not have a strong preference either way. This was the only case where I thought I needed something like removePipe.

ct-gdf avatar Jun 03 '24 23:06 ct-gdf

Okay. Then let's wait for more feedback from other developers to make a good decision in the long run. Feel free to create a new issue as I plan to close this one.

fabian-hiller avatar Jun 04 '24 09:06 fabian-hiller

@fabian-hiller hey 👋 I've tested latest version and would like you to check if object with rest is working correctly with omit:

playground

key1 and key3 are omitted but still are in result output (but type is correct w/o them). I'd expect key2 as the only key in result, same as plain object works.

vladshcherbin avatar Jun 25 '24 15:06 vladshcherbin

Yes, omit is implemented similarly to TypeScript's Omit type, and I don't think we should change that. The problem is that the specified rest contains every key, including key1 and key3. This feature will probably require us to implement a new except method.

fabian-hiller avatar Jun 25 '24 15:06 fabian-hiller

@fabian-hiller it's very unfortunate it works like this, I'd never expect it to work like this same as other devs from the linked issues above.

The docs say:

Creates a modified copy of an object schema that does not contain the selected entries.

The result of above example clearly shows that the object still has all selected entries 😱

vladshcherbin avatar Jun 26 '24 12:06 vladshcherbin

Have a look at the source code in line 407. As written in the docs, omit just creates a copy and removes the specified entries: https://github.com/fabian-hiller/valibot/blob/main/library/src/methods/omit/omit.ts

fabian-hiller avatar Jun 26 '24 13:06 fabian-hiller

@fabian-hiller yes, I also expect omit to remove the entries

In the latest example from above I have input data:

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

now omit:

omit(objectWithRest, ['key1', 'key3'])

here I expect omit to remove specified key1 and key3, but it actually does nothing for objectWithRest, the result object is not changed.

that's why I'd consider it as a bug or a very unreliable method and at least add a note about this behavior in the docs - personally I find the outcome result surprising

vladshcherbin avatar Jun 26 '24 21:06 vladshcherbin

This is actually not the same issue I mentioned when I opened this issue - it works correctly now in latest version.

A new issue should be probably opened for tracking objectWithRest + omit combination and other devs awareness.

vladshcherbin avatar Jun 26 '24 22:06 vladshcherbin