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

implement inline optional for record #113

Merged
merged 10 commits into from
Mar 24, 2021

Conversation

artalar
Copy link
Contributor

@artalar artalar commented Oct 13, 2019

The better way, then Record({}).And(Partail({}))

@artalar artalar changed the title implement inline optional to record implement inline optional for record Oct 13, 2019
@pelotom
Copy link
Collaborator

pelotom commented Oct 19, 2019

I like the motivation behind this, but it changes the semantics slightly from TypeScript:

{ foo?: number }

is not the same as

{ foo: number | undefined }

The former says that foo can be present or not in the object, and if it is present it could be undefined. The latter says that a foo property must always be present, but its value could be undefined.

To do this the right way, I think we would need the Optional property not to be a Runtype, but instead a special branding type for use only in Record values (it doesn't make sense to talk about Optional(Number) as a type in isolation). And even then I'm not sure it's possible; it seems like it would require some type crazy syntax that doesn't currently exist e.g.

{
  [K in keyof O][O[K] extends Optional ? +? : -?]: O[K] extends Optional<infer T> ? T : O[K]
}

@artalar
Copy link
Contributor Author

artalar commented Oct 19, 2019

it doesn't make sense to talk about Optional(Number) as a type in isolation

Totally agree, but I did not find other ways...

And even then I'm not sure it's possible

But I already do it :)

example

The question is how the right API may look like... Current workaround by Record and Partial interaction looks weird(

@pelotom
Copy link
Collaborator

pelotom commented Oct 20, 2019

I think it's reasonable to make an alias for the T | undefined type, understanding that it's not the same as an optional property on a record. It doesn't need to be a new core runtype though; we could simply add a new unary operator on the Runtype interface, similar to Or and And:

  /**
   * Union this Runtype with another.
   */
  Or<B extends Runtype>(B: B): Union2<this, B>;

  /**
   * Intersect this Runtype with another.
   */
  And<B extends Runtype>(B: B): Intersect2<this, B>;

+ /**
+  * 
+  */
+ Optional: Runtype<Static<this> | undefined>;

implemented as

  A.Or = Or;
  A.And = And;
+ A.Optional = Union(A, Literal(undefined));

@yuhr
Copy link
Collaborator

yuhr commented Mar 21, 2021

it doesn't make sense to talk about Optional(Number) as a type in isolation

Some languages have an algebraic datatype called Option, so I don't think it's nonsense to use this wording for an alias of unionizing with undefined, even in isolation.

I'd go for the OP's approach, which is having Optional as a discrete runtype. My proposal is:

  1. If Optional(Number) (or Number.Optional) runtype is used in isolation, it works just as an alias for Number.Or(Undefined) whose static type is number | undefined
  2. If used in a Record, it works as genuinely an optional property, with its static type resulting in { foo?: number }
  3. If one wants to really get { foo: number | undefined }, an explicit unionization i.e. Number.Or(Undefined) is needed

Why we need it to be a discrete runtype is nothing else but the point 2. Record runtype should dynamically check if a field is Optional or not, so that it can be properly handled.

@yuhr
Copy link
Collaborator

yuhr commented Mar 23, 2021

Anyway, the PR branch is a bit stale to continue enhancement, so I'm going to update it.

@coveralls
Copy link
Collaborator

coveralls commented Mar 23, 2021

Coverage Status

Coverage increased (+0.03%) to 99.175% when pulling 6350445 on artalar:inline-optional-to-record into 917435d on pelotom:master.

@yuhr yuhr marked this pull request as ready for review March 23, 2021 20:41
@yuhr yuhr mentioned this pull request Mar 23, 2021
@yuhr
Copy link
Collaborator

yuhr commented Mar 23, 2021

I ended up having the shorthand syntax of Optional as like String.optional(). This is not much shorter than writing Optional(String) when you import Optional directly from runtypes, but if you use scoped import i.e. import * as rt from "runtypes", it would look better to write rt.String.optional() rather than rt.Optional(rt.String). I decided to decapitalize as per consistency with upcoming APIs such as .pick() and .omit() in #161 (I think also .And() and .Or() should be renamed and deprecated, off-topic though).

@artalar
Copy link
Contributor Author

artalar commented Mar 23, 2021

Awesome 👍

@yuhr yuhr merged commit fe12a47 into runtypes:master Mar 24, 2021
@antaranyan
Copy link

Nice addition, definitely useful. When it's hitting npm?

@yuhr
Copy link
Collaborator

yuhr commented Mar 26, 2021

Sorry for being late, it should be available in v5.2.0 now!

72636c added a commit to 72636c/runtypes-filter that referenced this pull request Mar 28, 2021
}),
);
```

If a record has keys which _must be present_ but can be null, then use
the `Record` runtype normally instead.
There's a difference between `Union(String, Undefined)` and `Optional(String)` iff they are used within a `Record`; the former means "_**it must be present**, and must be `string` or `undefined`_", while the latter means "_**it can be present or missing**, but must be `string` or `undefined` if present_".
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo on iff

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

Successfully merging this pull request may close these issues.

None yet

6 participants