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

fix: refine the types more, to improve type extends behaviour #234

Merged
merged 1 commit into from
Aug 27, 2020

Conversation

forivall
Copy link
Contributor

@forivall forivall commented Aug 25, 2020

See also: #233 (i pulled the doc comment from here), #232, #231

The old types prevented

interface MyEventListeners {
  print(message: string): void
}

class MyEmitter extends EventEmitter<MyEventListeners> {
  emit<Event extends keyof MyEventListeners>(
    event: keyof MyEventListeners
    ...args: Parameters<MyEventListeners[Event]>
  ) {
    // customizations
    return super.emit(event, ...args);
  }
}

from working properly, as typescript borked where it fails to match
the arguments types with impossible-to-reach types.

The smallest change to achieve this fix is to change the any[] in Arguments<...> to never.

to understand this change, understand that the reference to the Arguments type will never reach the any[] case from within the EventListener type mapping, since K already must be a keyof T once we're in the alternate(else, after the :) branch of T extends string | symbol, since EventNames in this case will be specifically keyof T.

Typescript just isn't smart enough to them since this situation runs in parallel (not actually parallelism, just like, conceptually) in the type checker, the invariant i've attempted to describe is coincidental; the causation doesn't flow from one to the other or vice versa.

And the ArgumentMap type will already handle the any[] type for when the value in the object isn't a function or array.

Hope you can understand this explanation well enough to merge this change! I've done my best, i could do better with a whiteboard.

Also, i don't like having the type name Arguments because it's too close (IMO) to the builtin IArguments, so I felt it worthwhile to just remove that type, keep it smaller.

The old types prevented

interface MyEventListeners {
  print(message: string): void
}

class MyEmitter extends EventEmitter<MyEventListeners> {
  emit<Event extends keyof MyEventListeners>(
    event: keyof MyEventListeners
    ...args: Parameters<MyEventListeners[Event]>
  ) {
    // customizations
    return super.emit(event, ...args);
  }
}

from working properly, as typescript borked where it fails to match
the arguments types with impossible-to-reach types.
@lpinca
Copy link
Member

lpinca commented Aug 26, 2020

cc: @gfmio

@lpinca lpinca merged commit 8f8a7e0 into primus:master Aug 27, 2020
@lpinca
Copy link
Member

lpinca commented Aug 27, 2020

Thank you.

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

Successfully merging this pull request may close these issues.

2 participants