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

Conditional types #29

Closed
lax4mike opened this issue May 4, 2023 · 3 comments
Closed

Conditional types #29

lax4mike opened this issue May 4, 2023 · 3 comments

Comments

@lax4mike
Copy link
Contributor

lax4mike commented May 4, 2023

Hello!

I've been messing around with my example and I think I've narrowed the issue down to the fact that the following doesn't work:

expectType<string>(R.defaultTo('This file', undefined));

Here are the type definitions:

export function defaultTo<T, U>(a: T, b: U | null | undefined): T | U;
export function defaultTo<T>(a: T): <U>(b: U | null | undefined) => T | U;

I think the problem stems from the result type being T | U, which in this case would include undefined. I think we need to somehow use conditional types or something to make the types more aware about how the function behaves.

I assume this would be a problem for a bunch of other types too, like propOr, etc.

I don't know much about conditional types and I'm going to read those docs and do more research tomorrow, but I just wanted to post this to see if anyone else has any insight into this issue!

@Harris-Miller
Copy link
Collaborator

There are a few things going on here, first...

// `string` is not correct here
expectType<string>(R.defaultTo('This file', undefined)); // error

// `string | undefined` is
expectType<string | undefined>(R.defaultTo('This file', undefined)); // ok!

// `undefined` is taken literally above, no differently than how the return type is `string | number` for this
expectType<string | number>(R.defaultTo('This file', 123)); // ok!

Your typescript playground example is much more interesting though

R.compose(
  R.defaultTo("This file"), // :(
  R.nth(1),
  R.match(/.*\.(.*)/),
  R.propOr("", "name")
)

The main error that you get is

Argument of type '<T extends string | readonly any[]>(list: T) => (T extends (infer E)[] ? E : string) | undefined' is not assignable to parameter of type '(a: string[]) => U | null | undefined'.
      Type 'string | undefined' is not assignable to type 'U | null | undefined'.
        Type 'string' is not assignable to type 'U'.
          'U' could be instantiated with an arbitrary type which could be unrelated to 'string'.

I'm pretty sure that that error is specifically because the function that nth returns returns (T extends (infer E)[] ? E : string) | undefined. And the problem here is that the actual return of that returned function is not something that U can determine because of the infer

If you remove R.nth it works

R.compose(
  R.defaultTo("This file"), // 
  R.match(/.*\.(.*)/),
  R.propOr("", "name")
)

Once called, the return type is now string | string[] as expected. This is because R.match ends with the strict type string[]

See playground

This is just one of those situations where typescript inference breaks down. The only way I know how to fix this is to add another overload to defaultTo that lets you set T and U yourself. See playground

export function defaultTo<T, U>(a: T, b: U | null | undefined): T | U;
// new overload that lets you set `U` ahead of time
export function defaultTo<T, U>(a: T): (b: U | null | undefined) => T | U;
export function defaultTo<T>(a: T): <U>(b: U | null | undefined) => T | U;

@lax4mike
Copy link
Contributor Author

lax4mike commented May 4, 2023

Cool! That's interesting about the nth. I'll dig into that in a bit. Right now, I'd like to focus on defaultTo because I think the type definition is wrong.

expectType<string | undefined>(R.defaultTo('This file', undefined)); // << incorrect

If I'm not mistaken, the above is incorrect. Given a default value of 'This file', defaultTo will never return undefined. The correct return type for this is string.

I've come up with some new definitions using conditional types that I think satisfy the correct behavior of defaultTo:

export function defaultTo<Default, Value>(a: Default, b: Value): (Value extends (null | undefined) ? Default | NonNullable<Value> : Value);
export function defaultTo<Default>(a: Default): <Value>(b: Value) => (Value extends (null | undefined) ? Default | NonNullable<Value> : Value);

Here are some tests for defaultTo that pass:

expectType<string>(defaultTo('default value', undefined));

expectType<string>(defaultTo(0, 'two'));
expectType<number>(defaultTo('zero', 2));

const numberOrUndefined = Math.random() < 0.5 ? 0 : undefined;
expectType<string | number>(defaultTo('default value', numberOrUndefined));

This seems to be working correctly. Does this look correct to you?

(Here's the code if you want to take a look)

@Harris-Miller
Copy link
Collaborator

@lax4mike #30 fixed this issue, correct? Can we close this?

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

2 participants