microsoft / TypeScript-DOM-lib-generator

Tool for generating dom related TypeScript and JavaScript library files
Apache License 2.0
620 stars 421 forks source link

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

Open sandersn opened 6 years ago

sandersn commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

//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 commented 6 years ago

So TS now automatically converts unions into typedefs? Interesting.

RyanCavanaugh commented 6 years ago

@saschanaz only for display purposes

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

saschanaz commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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 commented 5 years ago

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