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

TimerHandler should not be an alias for string | Function #559

Open
sandersn opened this issue Aug 2, 2018 · 9 comments
Open

TimerHandler should not be an alias for string | Function #559

sandersn opened this issue Aug 2, 2018 · 9 comments
Assignees

Comments

@sandersn
Copy link
Member

sandersn commented Aug 2, 2018

Because of global type alias interning, the global type TimerHandler = string | Function means any string | Function union gets called TimerHandler. This makes (for example) lodash errors and quick info even more confusing.

Edit: here is an example of the confusing error:

function f(code: string | Function) {
  code = typeof code === 'string' ? new Function(code) : code;
  // etc...
}
f(null)

Argument of type "null" is not assignable to parameter of type 'TimerHandler'.

@mhegazy
Copy link
Contributor

mhegazy commented Aug 2, 2018

This is actually coming from the webidl, so there is not a simple way to remove these. do you want Function to be sometime different?

@mhegazy
Copy link
Contributor

mhegazy commented Aug 2, 2018

//CC @saschanaz

So the proposal is to flatten unions of primitives when we are emitting them.. so string | Function would be inlined in all usage sites instead of using the alias.

@saschanaz
Copy link
Contributor

So TS now automatically converts unions into typedefs? Interesting.

@RyanCavanaugh
Copy link
Member

@saschanaz only for display purposes

We might be able to do a sneaky workaround like string | Function | never ?

@saschanaz
Copy link
Contributor

saschanaz commented Aug 4, 2018

IMO one of the core issues is the automatic union-typedef conversion for display purposes:

  1. A user cannot easily use "Go to definition" to see what's going on.
  2. Even after we remove TimerHandler, a third party non-module type definition may insert a similar union of primitives, and then the same confusing error.

Edit:

Anyway I'm not a fan for those mini-typedefs as I think they are for spec authors rather than devs. If we remove them, will we also remove non-union typedefs e.g. typedef boolean GLboolean from WebGL?

@sandersn
Copy link
Member Author

sandersn commented Aug 6, 2018

@saschanaz A few replies:

  1. What goto-def scenario are you thinking of? I don't think that string | Function needs much explanation beyond what would be on the @param tag. Same for many of the other typedefs.

  2. 3rd parties may indeed insert similar unions, but at least the damage is contained to users of those packages. Nearly everybody uses the DOM.

    It does mean that we should perhaps make a sweep of definitely typed to make sure that commonly used packages like jquery, react, lodash do not define small, common type aliases.

  3. I think only unions have this behaviour because it's a combination of union interning and type alias display. Union interning means that string | Function always refers to the same type, no matter how many different places it's used. And types maintain a pointer to their type alias name -- but only one name per type. So if string | Function is seen in the alias type TimerHandler = string | Function, and all uses of string | Function are exactly the same type, that type always prints as "TimerHandler".

    Also note that boolean actually is a union (true | false), so GLboolean should also be removed.

@saschanaz
Copy link
Contributor

saschanaz commented Aug 17, 2018

@sandersn Oops, sorry for replying too late.

What goto-def scenario are you thinking of?

Users may confused because the compiler is using the names the user didn't write, so they may want to find where the name is from, by goto-def, Control+F, or anything.

I think the message itself is confusing here. Can we do something like:

Error: Argument of type "null" is not assignable to parameter of union type "string | Function" (aliased as "TimerHandler")

This way unions won't confuse users too much.

Edit: I just posted a comment and it unassigned a member 😮

@saschanaz
Copy link
Contributor

saschanaz commented Oct 30, 2018

@sandersn So this is again a potential problem in #590.

I'm still not sure it will be intuitive to have GLint but not GLboolean, ScrollLogicalPosition but not ScrollBehavior, etc. Is revising the error message impossible?

@saschanaz
Copy link
Contributor

Is this still the case? I can't reproduce this in TS3.3.

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

4 participants