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

Allow to use null as discriminator for tagged union #24193

Closed
4 tasks done
KSXGitHub opened this issue May 17, 2018 · 10 comments · Fixed by #27695
Closed
4 tasks done

Allow to use null as discriminator for tagged union #24193

KSXGitHub opened this issue May 17, 2018 · 10 comments · Fixed by #27695
Labels
Committed The team has roadmapped this issue Fixed A PR has been merged for this issue Help Wanted You can do this Suggestion An idea for TypeScript

Comments

@KSXGitHub
Copy link
Contributor

KSXGitHub commented May 17, 2018

Search Terms

Suggestion

Right now, even with strictNullChecks is set to true, null cannot be used as a discriminator for tagged union.

Use Cases

Recently, I made a pull request for @types/node in which I let spawnSync returns a tagged union with error property as discriminator.

Examples

interface WithError {
    error: Error
    data: null
}

interface WithoutError<Data> {
    error: null
    data: Data
}

type DataCarrier<Data> = WithError | WithoutError<Data>

// MAIN FOCUS:
//   TypeScript should be able to deduce error's and data's
//   types in the following function 
function test<Data>(carrier: DataCarrier<Data>) {
    if (carrier.error === null) { // `error` property as disciminator
        const error: null = carrier.error // should work
        const data: Data = carrier.data // should work
    } else {
        const error: Error = carrier.error // should work
        const data: null = carrier.data // should work
    }
}

Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript / JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. new expression-level syntax)
@MartinJohns
Copy link
Contributor

MartinJohns commented May 17, 2018

Data is a generic, it could be any type. What should happen when it's of the type string | null? Then you have the same possible value (null) in both cases.

@KSXGitHub
Copy link
Contributor Author

KSXGitHub commented May 17, 2018

@MartinJohns If Data is string | null, then data is of type string | null only in the if-branch, while in the else-branch, data could only be null. I see no problem with this.

Edit: I made a mistake in if-condition, it should be carrier.error === null. Sorry for the confusion.

@ajafff
Copy link
Contributor

ajafff commented May 17, 2018

@KSXGitHub I think you meant carrier.error === null in the condition? That narrows error correctly, but doesn't discriminate data.

@KSXGitHub
Copy link
Contributor Author

@ajafff Oh, thanks. I made a mistake.

@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels May 17, 2018
@DanielRosenwasser
Copy link
Member

Isn't this the same as #24479?

@DanielRosenwasser
Copy link
Member

Oh whoops, I'm the duplicate.

@AnyhowStep
Copy link
Contributor

Just copy-pasting a use-case from my issue, #25447 ,

interface A {
  f?: number;
  bar: string; 
}

interface B {
  f: number; 
}
declare const x: A | B;
if (x.f == undefined) {
  //Expected:
  //'x' is narrowed to type 'A'
  //Actual:
  //Property 'bar' does not exist on type 'A | B'.
  //Property 'bar' does not exist on type 'B'.
  console.log(x.bar);
} 

Playground Link

@simonbuchan
Copy link

simonbuchan commented Jul 24, 2018

Seems like this is the feature required to type react-spring's native prop sensibly, though in a type inference usage, rather than guard usage?

Using the standard animated Spring (i.e. where the render prop is invoked for each new frame and updates using react), where the native property is not set or set to false, the style parameter should be inferred directly from from and to. e.g. { background: string, width: number }:

import { Spring } from "react-spring";

<Spring
  from={{ background: 'blue', width: 0 }}
  to={{ background: 'red', width: 100 }}
>
  {style => (
    <div style={style}>React animation</div>
  )}
</Spring>

With native (i.e. directly to DOM, skipping react) animation, style inferred as a mapping of the types of from and to to an object of react-spring AnimatedValues, e.g. { background: AnimatedValue<string>, width: AnimatedValue<number> }, and animated provides components that accept AnimatedValues for their props (simplifying a bit):

import { Spring, animated } from "react-spring";

<Spring
  native
  from={{ background: 'blue', width: 0 }}
  to={{ background: 'red', width: 100 }}
>
  {style => (
    <animated.div style={style}>Native animation</animated.div>
  )}
</Spring>

At the moment I can type Spring (and the various other components with this pattern) correctly as either react or native animation, and as a union of these it works if native is explicitly provided (i.e. native={false} for the react animation case), but when native is just omitted as expected, TS seems to assume that it could be either, and thus refuses to infer style, meaning it gets any.

This isn't specific to JSX, though, here's a simplified example of this case:

declare class AnimatedValue<T> {
    interpolate<U>(map: (value: T) => U): AnimatedValue<U>;
}

type DynamicProps<T> = { native?: false, value: T, render: (value: T) => string };
type NativeProps<T> = { native: true, value: T, render: (value: AnimatedValue<T>) => AnimatedValue<string> };
type AutoProps<T> = DynamicProps<T> | NativeProps<T>;

declare function renderAuto<T>(props: AutoProps<T>): string;

// Works, x is number:
renderAuto({ native: false, value: 123, render: x => x.toFixed() });
// x is any, so error under --noImplicitAny
renderAuto({ /* native: false, */ value: 123, render: x => x.toFixed() });
// works, ax is AnimatedValue<number>
renderAuto({ native: true, value: 123, render: ax => ax.interpolate(x => x.toFixed()) });

@RyanCavanaugh RyanCavanaugh added Committed The team has roadmapped this issue and removed In Discussion Not yet reached consensus labels Aug 23, 2018
@RyanCavanaugh RyanCavanaugh added this to the Future milestone Aug 23, 2018
@RyanCavanaugh RyanCavanaugh added the Help Wanted You can do this label Aug 23, 2018
@RyanCavanaugh
Copy link
Member

Accepting PRs or we'll take care of it at some future point

@jack-williams
Copy link
Collaborator

PR up at #27631

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Committed The team has roadmapped this issue Fixed A PR has been merged for this issue Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants