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

pick, omit, require & partial should use arrays instead of objects #1563

Closed
janpaepke opened this issue Nov 16, 2022 · 15 comments
Closed

pick, omit, require & partial should use arrays instead of objects #1563

janpaepke opened this issue Nov 16, 2022 · 15 comments

Comments

@janpaepke
Copy link

janpaepke commented Nov 16, 2022

I was wondering what the rationale behind pick, omit, require and partial expecting objects is?

Each value is optional and can only be true, so using

.pick(['foo', 'bar'])

feels more sensible than

.pick({ foo: true, bar: true })

There is another reason than convenience:
Currently you can (accidentally) add properties, which do not exist on the original object.
With an array you could ensure type safety.

The signature would look like this

pick<Mask extends Array<keyof T>>(mask: Mask): ZodObject<Pick<T, Extract<keyof T, keyof Mask>>, UnknownKeys, Catchall>;

If you agree I could offer to implement this.
Either in a backwards compatible way (also accepting objects, using overloads) or as a breaking change.

@igalklebanov
Copy link
Contributor

There is another reason than convenience:
Currently you can (accidentally) add properties, which do not exist on the original object.
With an array you could ensure type safety.

Check out #1564, it's fixable.

@janpaepke
Copy link
Author

janpaepke commented Nov 17, 2022

Hi Igal,
thank you for your response and contribution for making unknown properties fail.
I am still wondering though about the rationale.
Why would we be using an object here? An array makes way more sense to me.

@BenWaNH
Copy link

BenWaNH commented Nov 18, 2022

I agree with @janpaepke
We'll shall always set prop to true
And it will be more easier to use, when receiving an array of pick props that we'll can set in pick method directly instead parse in object.

@janpaepke
Copy link
Author

updated title and description, to include require and partial

Again: I am willing to add a PR for this, but want to understand the reason for using an object in the first place.

@janpaepke janpaepke changed the title pick / omit using arrays instead of objects pick, omit, require & partial should use arrays instead of objects Nov 18, 2022
@janpaepke
Copy link
Author

@colinhacks May I ask for your insight?

@capaj
Copy link

capaj commented Dec 14, 2022

@janpaepke Prisma uses the same object notation for selecting fields.
IMHO it is easier to type correctly in typescript. I don't mind the extra verbosity, but you're right that doing pick({field: false}) doesn't make sense so using a Boolean might be a bit overkill.

Why not have both?

@colinhacks
Copy link
Owner

Too much of a footgun. Many many people will inevitably try this:

const keys = ['a', 'b'];

const schema = z.object({
  a: z.string(),
  b: z.string(),
  c: z.string()
}).pick(keys); // bad

Which is bad because Zod can't properly type the resulting schema, since the type signature of keys has collapsed to string[].

A similar problem exists for z.enum and it's caused a lot of headaches both for users and maintainers.

@Not-Jayden
Copy link

@colinhacks wondering if the const modifier might make this more feasible in the future? microsoft/TypeScript#51865

@colinhacks
Copy link
Owner

colinhacks commented Dec 15, 2022

This syntax is already implementable without const, that's not the problem. It's just a footgun that will cause users frustration. This is the part that makes this syntax a footgun, and const doesn't fix that unfortunately.
Screenshot 2022-12-14 at 7 28 37 PM

@Not-Jayden
Copy link

Ah yep I misunderstood that part of the PR, my apologies.

@janpaepke
Copy link
Author

janpaepke commented Dec 20, 2022

Hi @colinhacks,

thanks for the feedback, that does make sense and I hadn't thought of that.
I will argue though that I still think it would be sensible to do.

Starting from your example: This wouldn't fail zod parsing, it would fail one step before, provided the function expects an array of the keys of T, as I wrote in my example in the original issue.

Then it would fail with something like

Argument of type 'string[]' is not assignable to parameter of type '(keyof T)[]'.
  Type 'string' is not assignable to type 'keyof T'.

This would point the user to the root of the issue (the type ambiguity of the provided array) and he can solve it in one of two ways:
A) providing the array inline .pick(['a', 'b'])
B) using const assertion: const keys = ['a', 'b'] as const; making the array readonly.

Use this to test:

type O = {
	a: number,
	b: string,
	c: boolean
}

const x = ['a', 'b'] as const

const pickTest = <T extends keyof O>(x: readonly T[]) => void x

pickTest(x);

Do you still think it is an inevitable footgun, or was I able to get you to at least consider to support both syntaxes?

@janpaepke
Copy link
Author

@colinhacks I don't want to bother you with this, just make sure I understand your reasoning... Could you comment?

@tombburnell
Copy link

I noticed that the input keys to schema.omit(...) are not type checked (unless user error or my behalf). The following does not warn for me that "item" is spelt incorrectly.

schema.omit( { iteem: true }); // does not warn.

So I created the following omitKeys() function that creates the omit params { key => true } from a type-checked list:

function omitKeys(...keys: Array): { [key: string]: true } {
return Object.fromEntries(keys.map(key => [ key, true]));
}

Combined with an inferred interface, this allows passing type-checked input args as so..

export type ISchema = z.infer;
const reduced = schema.omit(omitKeys("iteem")); // this now does warn.

Ideally you could do something like the following and the interface inference just happens behind the scenes.

const reduced = schema.omit("key1", "key2");

But that might be hard so maybe you could add this method for convenience instead?

@tombburnell
Copy link

I've discovered my "solution" breaks the inferred interface of the resulting schema which just shows {} when using the interface.

Also if I only put one key in, it type checks it, but if I have multiple and one is fine, it does not. What could be happening here?

const reduced = schema.omit({
"optionn": true, // warns
});

const reduced = schema.omit({
"optionn": true, // does not warn, status is valid
"status": true,
});

const reduced = schema.omit({
"optionn": true, // warns - neither are valid
"statuss": true,
});

I'm using webstorm
"zod": "^3.22.4",
"typescript": "^5.1.6"

@tombburnell
Copy link

I see there is a ticket for this already.. #2607

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

No branches or pull requests

7 participants