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

Optional keys respecting exactOptionalPropertyTypes #385

Closed
tpetry opened this issue Jan 24, 2024 · 43 comments
Closed

Optional keys respecting exactOptionalPropertyTypes #385

tpetry opened this issue Jan 24, 2024 · 43 comments
Assignees
Labels
enhancement New feature or request priority This has priority

Comments

@tpetry
Copy link

tpetry commented Jan 24, 2024

TypeScript 4.4 added exactOptionalPropertyTypes as a more strict setting to better handle optional object keys. This is currently not supported in valibot.

import * as v from 'valibot';

interface Data {
    key?: string
}

const schema = v.object({
    key: v.optional(v.string()),
});

// TS2375: Type  { key?: string | undefined; }  is not assignable to type Data  with 'exactOptionalPropertyTypes: true'.
const value: Data = v.parse(schema, {});

It would be great if there is a way to define optional attributes that do not automatically add undefined to the type. zod has the same problem (colinhacks/zod#635, colinhacks/zod#1510), while io-ts works correctly but is hard to use.

@fabian-hiller fabian-hiller self-assigned this Jan 24, 2024
@fabian-hiller fabian-hiller added the enhancement New feature or request label Jan 24, 2024
@fabian-hiller
Copy link
Owner

Thank you for creating this issue. Do you have any ideas how we could improve our API to support this? One option is to have two different schema function for both use cases and another to stick with optional and make it configurable.

@tpetry
Copy link
Author

tpetry commented Jan 24, 2024

Both sounds good to me.

@fabian-hiller
Copy link
Owner

What would you name the function or configuration option? Feel free to provide sample code for your dream API. Maybe I can make it happen.

@tpetry
Copy link
Author

tpetry commented Jan 25, 2024

I first thought just adding a simple boolean flag to the optional function:

const schema = object({
    key: optional(string(), true),
});

But that would be weird when the schema is just some string with validations:

const schema = optional(string([email(), minLength(8)]), true));

I guess a new function that only makes sense for objects makes the most sense. The name partial was choosen because of the Partial<T> utility type.

const schema = object({
    key: partial(string()),
});

Other synonyms could be:

  • strictOptional(): as it is a stricter optional setting
  • missing(): the attribute can be missing in the object/interface

@fabian-hiller
Copy link
Owner

Thanks for your feedback. I will think about it, but do not expect a quick response as my focus is currently on other areas of the library. partial already exists to make an object's entries optional.

@ComradeVanti
Copy link

This issue is one of the main reasons I stopped using zod. If valibot could get it done that would be really neat.

@fabian-hiller
Copy link
Owner

Can you explain why this feature is important to you? What are the problems with the current approach?

@ComradeVanti
Copy link

ComradeVanti commented Jul 28, 2024

@fabian-hiller Sure. In my case I have an existing external type like this:

type Maintainer = { name?: string, url?: string, email?: string }

I also have exactOptionalPropertyTypes enabled in my project for additional type safety.

I want to accurately represent this type using a schema. But currently that is not possible (as I understand it) because using v.optional I will get

type Maintainer = { name?: string | undefined, url?: string | undefined, email?: string | undefined }

This will cause issues in two places. First I want to enforce that my schema matches the TS type and so use

const maintainerSchema = z.object({ ... }) satisfies v.BaseSchema<Maintainer, Maintainer, v.BaseIssue<unknown>>

This does not work because the schema is not assignable to the TS type. The issue is the explicit undefined.

The same issue will also prevent me from assigning objects of type v.InferOutput<typeof maintainerSchema> to instances of Maintainer in my code. So if I get a Maintainer object from an external source and validate it using maintainerSchema I will not be able to then treat it as an instance of Maintainer in my code, which somewhat defeats the purpose of the validation.

Hope this makes it clear :)

@tpetry
Copy link
Author

tpetry commented Jul 28, 2024

Thats exactly the issue I have. At the moment I solved it by manually type-checking the validated valibot object that no undefined value is used when the property exists. But for some complex objects thats hundreds of lines of code to check dozens of properties.

@ComradeVanti
Copy link

ComradeVanti commented Jul 28, 2024

Here is the workaround code I now use in zod in case anyone is interested. I'm sure it can be adapted to valibot too.

type RemoveExplicitUndefined<T> = T extends undefined
  ? never
  : T extends z.BRAND<string>
  ? T
  : T extends object
  ? { [K in keyof T]: RemoveExplicitUndefined<T[K]> }
  : T;

export function removeExplicitUndefined(value: undefined): never;
export function removeExplicitUndefined<T>(
  value: T
): RemoveExplicitUndefined<T>;
/**
 * Recursively removes all instances of explicit undefined from a value.
 * This mostly gets rid of explicit undefined properties in objects.
 * @param value The value.
 * @throws Error if the passed value is undefined.
 */
export function removeExplicitUndefined(value: unknown) {
  if (value === undefined) {
    throw new Error("Cannot remove undefined from undefined!");
  }
  if (value === null || typeof value !== "object") {
    return value;
  }
  return Object.fromEntries(
    Object.entries(value)
      .filter(([, v]) => v !== undefined)
      .map(([key, v]) => [key, removeExplicitUndefined(v)])
  );
}


// Usage
const tsValidValue = removeExplicitUndefined(schemaValidValue);

@fabian-hiller
Copy link
Owner

Thanks for all the feedback. According to Colin (maintainer of Zod) it is possible to detect if exactOptionalPropertyTypes is enabled. This would allow us to change the type implementation of optional to support this configuration. However, it might be better to add a new schema. This way you can control if you want only ? or also undefined when exactOptionalPropertyTypes is enabled. We could name this schema for example missing (idea by @tpetry). What do you think?

@ComradeVanti
Copy link

ComradeVanti commented Aug 2, 2024

missing sounds good to me. Thanks for looking into this :)

@fabian-hiller
Copy link
Owner

fabian-hiller commented Aug 2, 2024

Another name that came to mind is maybe. This describes it better than missing, since the value is only maybe available. But there is a drawback: v.maybe(v.string()) can be confusing because it is not clear what "maybe" refers to. What do you think?

@tpetry
Copy link
Author

tpetry commented Aug 2, 2024

Maybe is perfect. Because the value may maybe there - and its the exact definition of the value that may be there.

@fabian-hiller
Copy link
Owner

fabian-hiller commented Aug 2, 2024

There is one more problem that came to my mind. The optional schema adds undefined to the output type of the wrapped schema. For maybe to work correctly with exactOptionalPropertyTypes, it should not add undefined to the output type. The drawback of this is that the output type is now misleading when using maybe as a standalone schema without object, because undefined passes maybe but this is not represented in its output type, leading to potential runtime errors.

const OptionalSchema = v.optional(v.string()) // string | undefined
const MaybeSchema = v.maybe(v.string()) // string <--

const ObjectSchema = v.object({
  foo: OptionalSchema,
  bar: MaybeSchema
}); // { foo?: string | undefined; bar?: string }

@tpetry
Copy link
Author

tpetry commented Aug 2, 2024

May an API like this:

v.maybe(v.object({
  foo: OptionalSchema,
  bar: MaybeSchema
}, ['bar']);

But yes. It looks horrible compared to the clean and easy API currently.

Is it a big problem if a Maybe<T> type would only have an effect when passed to a v.object?

@fabian-hiller
Copy link
Owner

A fix could be to add undefined to the output type of maybe and filter/remove undefined when using maybe with object, but there is another problem. Someone might write v.maybe(v.undefined()) or a similar scheme and removing undefined would lead to unexpected behavior.

@fabian-hiller
Copy link
Owner

fabian-hiller commented Aug 2, 2024

But yes. It looks horrible compared to the clean and easy API currently.

How would this API solve the problem?

Is it a big problem if a Maybe<T> type would only have an effect when passed to a v.object?

Not sure. The problem is that each schema is independent and should not produce unexpected behavior that can lead to runtime errors when using it without a specific schema such as object.

@tpetry
Copy link
Author

tpetry commented Aug 2, 2024

How would this API solve the problem?

It would work similar to v.pick: From an existing object (that is not using optional) mark some attributes as allowed to be missing. It doesn't change the attributes types but adds the missing information to the type & validation.

Not sure. The problem is that each schema is independent and should not produce unexpected behavior that can lead to runtime errors when using it without a specific schema such as object.

When Maybe<T> is just a wrapper type it wouldn't have any runtime effects? Only when passed to v.object it could be unwrapped and set correctly with the missing information on the object type.

@fabian-hiller
Copy link
Owner

Can you explain the Maybe<T> type. How would you write your schema with it?

@tpetry
Copy link
Author

tpetry commented Aug 2, 2024

// should be recognized on the type-level like: type Maybe<string> = string 
const myStr = v.maybe(v.string()); // Maybe<string>

v.object(
  str: myStr,
); // object { str?: string }

I don't know whether this works. I am struggling with those advanced TS types all the time.

My Idea was to make Maybe<T> a simple wrapper type. And it gets unwrapped and used by the 'missing' logic in v.object. Everything else would just handle a Maybe<T> as T.

@fabian-hiller
Copy link
Owner

This is what I was talking about in this comment. It can work, but there are edge cases. Unwrapping is one solution. The problem here is that we can't know how deeply maybe is wrapped in other schemas. Another solution would be to extract undefined when we detect maybe, but again it is hard to detect maybe when it is wrapped in other schemas like nullable or nonNullable. These are just edge cases, but I want to at least try to get it right.

@fabian-hiller
Copy link
Owner

As written here, I am toying with the idea of simplifying the question mark handling. This would also allow us to fix this issue.

@fabian-hiller
Copy link
Owner

I have been trying to implement and support exactOptionalPropertyTypes. Unfortunately, I encountered a few TS problems that I need to investigate first.

@incompletude
Copy link

same here. need to match exact optionals.

@fabian-hiller fabian-hiller added the priority This has priority label Aug 23, 2024
@fabian-hiller
Copy link
Owner

Good news! exactOptionalPropertyTypes will be fully supported in the next version. 🎉

@fabian-hiller
Copy link
Owner

v0.39.0 is available

@alecmev
Copy link

alecmev commented Aug 24, 2024

Is there a schema like v.nullable but for undefined? Beside v.union([..., v.undefined()])? Until now I was using v.optional + Required<v.InferInput<typeof schema>>.

@fabian-hiller
Copy link
Owner

fabian-hiller commented Aug 24, 2024

Not yet, undefinable sounds wrong. 😅 We thought about adding maybe for "optional" keys but they call it optional everywhere in the TypeScript docs. That's why I choose to change the current behaviour for optional and nullish when the exactOptionalPropertyTypes config is enabled.

@alecmev
Copy link

alecmev commented Aug 25, 2024

I don't mind the name, just want something short for | undefined 😉 undefinable is unusual but it's a real word, and there's some prior art: option-t, tsdef. One downside is that it means something else in ts-toolbet.

@fabian-hiller
Copy link
Owner

I am unsure about undefinable because the behaviour does not match the official meaning of the word. Another option would be undefinedable, which would fit the pattern of nullable, but it is a very long name. maybe is another option, but I don't like the fact that it's not entirely clear what "maybe" refers to when you write maybe(string()). Some people will assume string | unknown and others string | undefined. Do you have other ideas or feedback?

@alecmev
Copy link

alecmev commented Aug 25, 2024

I think undefinable is still okay, as it was an intuitive first choice for you and several other people on the internet. Another option is withUndefined, as there's already a precedent for with in objectWithRest and tupleWithRest. It's as long as undefinedable, but easier to spell/pronounce and probably remember.

@fabian-hiller
Copy link
Owner

I may introduce an undefinedable schema with the next release. I think it fits our API best at the moment. If we get more ideas or feedback on its name, I will rename it to a better one before v1.

@tpetry
Copy link
Author

tpetry commented Aug 26, 2024

I've tested the new release and it works as expected. Thanks ❤️

I started with the smallest module and was able to remove a lot of logic as I can now return the validation result directly and don't have to check for those undefined values anymore. The code is now basically reduced to just setting up valibot validations and running them. No more post-processing needed! 🔥

@fabian-hiller
Copy link
Owner

fabian-hiller commented Aug 26, 2024

@alecmev should undefinedable also add a question mark to object entries to mark them as optional? If yes, only if exactOptionalPropertyTypes is disabled or always.

@alecmev
Copy link

alecmev commented Aug 26, 2024

I can’t comment on what makes sense for the API overall, but I personally just want sugar for v.union([…, v.undefined()]) 😉 So, my answer to the above would be “preferably never”. To help understand why: I’m validating environment variables, and I want to allow undefined, but make sure that all keys are filled out, as to make sure that some variable isn’t forgotten.

@fabian-hiller
Copy link
Owner

The validation of optional hasn't changed. We just changed the types when using optional for the entries of any object schema, or does your comment only refer to the inferred types?

@alecmev
Copy link

alecmev commented Aug 26, 2024

Yep, I use satisfies v.InferInput.

@fabian-hiller
Copy link
Owner

Thank you very much for your feedback! I will think about it and probably get back to you tomorrow or the day after.

fabian-hiller added a commit that referenced this issue Aug 27, 2024
@fabian-hiller
Copy link
Owner

undefinedable will be available in the next release. No question mark is added to object entries.

@alecmev
Copy link

alecmev commented Aug 29, 2024

Great, thank you 👍

@fabian-hiller
Copy link
Owner

v0.40.0 is available 🚀

@incompletude
Copy link

@fabian-hiller thanks for all the work. great job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority This has priority
Projects
None yet
Development

No branches or pull requests

5 participants