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

EventEmitter types still broken on Typescript 3.9 RC #225

Closed
sandersn opened this issue May 11, 2020 · 12 comments · Fixed by #226
Closed

EventEmitter types still broken on Typescript 3.9 RC #225

sandersn opened this issue May 11, 2020 · 12 comments · Fixed by #226

Comments

@sandersn
Copy link

Typescript 3.9, which should be out some time this week, still has an error when compiling

Here's my tsconfig:

{
    "compilerOptions": {
        "strict": true,
        "lib": ["esnext", "dom"],
        "types": []
    }
}

And my index.ts:

import eventemitter3 = require("eventemitter3");

The error is here:

  export type EventListener<
    T extends ValidEventTypes,
    K extends EventNames<T>
  > = T extends string | symbol
    ? (...args: any[]) => void
    : (...args: ArgumentMap<T>[K]) => void; // error

"Type 'K' cannot be used to index type 'ArgumentMap'."

I can't tell what's going wrong from a quick glance at the types; there are too many layers of indirection here. My guess is that the compiler just can't find a declaration that says K is a legal key of ArgumentMap<T> — but I couldn't figure out what declaration would work.

Discovered in the Typescript nightly tests, where we compile our nightly against the latest shipped types of widely used packages: microsoft/TypeScript#38405

@gfmio
Copy link
Contributor

gfmio commented May 11, 2020

Thanks, @sandersn. I will have a look, but doesn't this point to a regression in TS 3.9, given that it seems to work in TS 3.8 (and below)?

@gfmio
Copy link
Contributor

gfmio commented May 11, 2020

@sandersn I just tried this with typescript versions 3.9.0-beta, 3.9.1-rc, 3.9.2-insiders.20200509 and 4.0.0-dev.20200511 and I cannot reproduce this error. Have you tried your test with the latest version eventemitter3@4.0.3?

@sandersn
Copy link
Author

@gfmio I can repro this by running

$ tsc index.d.ts

From the root of the eventemitter3 repo. Can you double-check that your globally installed version of typescript is 3.9 RC or higher? (I haven't tried with 3.9.0-beta, just 3.9.1-rc and 4.0.0-dev)

@sandersn
Copy link
Author

I'll take another look at the types and see why they broke in 3.9.

@gfmio
Copy link
Contributor

gfmio commented May 11, 2020

@sandersn I think that this is due the conditional type I'm using in ValidEventTypes:

  export type ValidEventTypes<T = any> = string | symbol | T extends {
    [K in keyof T]: any[] | ((...args: any[]) => void);
  }
    ? T
    : never;

  export type ArgumentMap<T extends object> = {
    [K in keyof T]: T[K] extends (...args: any[]) => void
      ? Parameters<T[K]>
      : T[K] extends any[]
      ? T[K]
      : any[];
  };

  export type EventListener<
    T extends ValidEventTypes,
    K extends EventNames<T>
  > = T extends string | symbol
    ? (...args: any[]) => void
    : (...args: ArgumentMap<T>[K]) => void;

As I'm removing string | symbol using the conditional type in EventListener, the problem lies with type ValidEventTypesWithoutStringOrSymbol<T = any> = T extends { [K in keyof T]: any[] | ((...args: any[]) => void); } ? T : never.

This conditional type represents an object with arbitrary keys whose keys are all any[] or (...args: any[]) => void). In ts3.8 and below, this type is compatible with object, but it is not in 3.9 and above.

@sandersn
Copy link
Author

The lack of errors in 3.8 is due the behaviour of T extends any in 3.9 -- it no longer suppresses errors but behaves like you actually wrote T extends unknown. This makes Typescript more sound in obscure cases. In the PR we discussed a mechanical workaround, but that just puts off the problem.

In this case T extends ValidEventTypes<any> is equivalent to any, so in EventListener (and everywhere else), ValidEventTypes is currently a roundabout way of saying any.

I'm trying to figure out a way to get rid of the error, but in the meantime, this is closer to your prose description of ValidEventTypes:

    export type ValidEventTypes = string | symbol | { [s: string]: any[] | ((...args: any[]) => void) };

@gfmio
Copy link
Contributor

gfmio commented May 11, 2020

@sandersn Thanks for the heads up. I've thought about it and can't we just do the following?

export type ValidEventTypes = string | symbol | Record<any, any[] | ((...args: any[]) => void)>;

@sandersn
Copy link
Author

Yes, that seems to work, although I'm suspicious, because it should be equivalent to my attempt, which still has the 3.9 error. I'm not sure why that is. @weswigham do you have any ideas?

@weswigham
Copy link

The Record over any also produces a numeric index signature, and, if related to another Record, specifically, will admit behavior similar to a symbol index signature (since mapped types are compared by their indexes as a whole).

@gfmio
Copy link
Contributor

gfmio commented May 11, 2020

So, the valid key types that the Record should really accept are string | symbol, since number is not officially supported by eventemitter3 afaik. However, if I put in anything other than any in the Record's key type argument, the type error returns.

@gfmio
Copy link
Contributor

gfmio commented May 11, 2020

I think I've found a solution by using Exclude.

  export type ValidEventTypes =
    | string
    | symbol
    | { [K in string | symbol]: any[] | ((...args: any[]) => void) };

  export type EventNames<T extends ValidEventTypes> = T extends string | symbol
    ? T
    : keyof T;

  export type ArgumentMap<T extends object> = {
    [K in keyof T]: T[K] extends (...args: any[]) => void
      ? Parameters<T[K]>
      : T[K] extends any[]
      ? T[K]
      : any[];
  };

  export type EventListener<
    T extends ValidEventTypes,
    K extends EventNames<T>
  > = T extends string | symbol
    ? (...args: any[]) => void
    : (...args: ArgumentMap<Exclude<T, string | symbol>>[K]) => void;

Can you verify that this works for you as well and that this won't cause any unintended side effects?

@sandersn
Copy link
Author

That works for me, and is basically the workaround discussed in the PR that introduced the breaking change, which means that it should be a pretty safe change.

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 a pull request may close this issue.

3 participants