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

Provide guidance for typings authors about order in union types #28044

Closed
rkirov opened this issue Aug 11, 2018 · 4 comments
Closed

Provide guidance for typings authors about order in union types #28044

rkirov opened this issue Aug 11, 2018 · 4 comments

Comments

@rkirov
Copy link

rkirov commented Aug 11, 2018

When writing a typing that has an union type, usually the authors put the least commonly used variant for last. Likely because it is the last thing they come up with, and seemingly the order in the union doesn't matter.

However, when there is a type error the error message down into the last option. This happens even with TS 3.0+ improved type errors. For example:

// typings
interface A {
  a: string;
  b: number
};

interface Other {
  o: number;
}

function f(x: A | Other): void;

// user
let x = {a: '', b: ''}; 
let y = f(x);

The error is:

  Type '{ a: string; b: string; }' is not assignable to type 'Other'.
    Property 'o' is missing in type '{ a: string; b: string; }'.

I see this issue very commonly with typings like angular.js and d3 and it is a major struggle for new-comers to TypeScript.

Usually the author of the typings has knowledge about this variant in the union is used more often, they should be guided how to write the typings to produce maximally userful errors.

That's said, as I was testing this locally, I couldn't figure out whether the declaration order matter (is interface A above interface Other) or just the union order (A | Other vs Other | A). It would be nice if TS team can say whether they would be comfortable with typings guide relying on the order here.

The same thing can be said about function overloads, but things are more complicated there because changing the order can affect not only erring code, but also code that currently typechecks.

/cc @DanielRosenwasser

@evmar
Copy link
Contributor

evmar commented Aug 11, 2018

Can you give some real-world examples of typings where this matters?

@rkirov
Copy link
Author

rkirov commented Aug 14, 2018

AngularJS typings:

let d: ng.IDirective = {
  link: (x: string) => {}
}

Error in TS 3.0.

a.ts:2:3 - error TS2322: Type '(x: string) => void' is not assignable to type 'IDirectiveLinkFn<IScope> | IDirectivePrePost<IScope> | undefined'.
  Type '(x: string) => void' has no properties in common with type 'IDirectivePrePost<IScope>'.

2   link: (x: string) => {}
    ~~~~

  node_modules/@types/angular/index.d.ts:2112:9
    2112         link?: IDirectiveLinkFn<TScope> | IDirectivePrePost<TScope>;
                 ~~~~
    The expected type comes from property 'link' which is declared here on type 'IDirective<IScope>'

The error mentions IDirectivePrePost<TScope> first, but in reality most authors use IDirectiveLinkFn<TScope>. I think if IDirectiveLinkFn<TScope> was mentioned in the error, the typing would have been more newcomer friendly. For large APIs like d3 or angularJS to user might not even know about the long-tail options like IDirectivePrePost<TScope>.

@DanielRosenwasser
Copy link
Member

We sort unions by declaration order, but I don't necessarily want to give prescriptive ideas on ordering because that feels like an implementation detail. You can feel free to re-order certain declarations, but I think you are touching on other issues in the TypeScript UX which we should fix.

Instead I'd rather focus on questions like: "how do we make the error message not bad here?"

Examples of where we've done work to elaborate better:

So can we do something similar here too. For example:

@rkirov
Copy link
Author

rkirov commented Aug 16, 2018

Handling it at the language level would be even better. I was skeptical at first, but indeed in both microsoft/TypeScript#26449 and microsoft/TypeScript#26450 it seems there is enough info for the compiler to infer a better path in the error than currently. Thanks for taking on those!

Closing this issue in favor of the Microsoft/TypeScript.

@rkirov rkirov closed this as completed Aug 16, 2018
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

3 participants