[node] Add generics to 'EventEmitter' #68313

malthe · 2024-01-24T17:55:47Z

Please fill in this template.

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Add or edit tests to reflect the change.
Follow the advice from the readme.
Avoid common mistakes.
Run pnpm test <package to test>.

Select one of these and delete the others:

If adding a new definition:

The package does not already provide its own types, or cannot have its .d.ts files generated via --declaration
If this is for an npm package, match the name. If not, do not conflict with the name of an npm package.
Create it with dts-gen --dt, not by basing it on an existing project.
Represents shape of module/library correctly
tsconfig.json should have noImplicitAny, noImplicitThis, strictNullChecks, and strictFunctionTypes set to true.

If changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes: Proposal/Discussion: NodeJS.EventEmitter should be generic #25302.
If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the package.json.

If removing a declaration:

If a package was never on Definitely Typed, you don't need to do anything. (If you wrote a package and provided types, you don't need to register it with us.)
Delete the package's directory.
Add it to notNeededPackages.json.

malthe · 2024-01-24T17:56:18Z

This fixes issue #25302 – see also discussion #55298.

typescript-bot · 2024-01-24T17:56:27Z

@malthe Thank you for submitting this PR!

This is a live comment which I will keep updated.

8 packages in this PR

newman — on npm, on unpkg
node-red — on npm, on unpkg
node — on npm, on unpkg
opossum — on npm, on unpkg
rdf-store-fs — on npm, on unpkg
sse — on npm, on unpkg
twitter — on npm, on unpkg
xml-flow — on npm, on unpkg

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

You can test the changes of this PR in the Playground.

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
✅ A DT maintainer needs to approve changes which affect more than one package

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 68313,
  "author": "malthe",
  "headCommitOid": "8838f077266af765677601d98af911c801996fa0",
  "mergeBaseOid": "2cf3fb47fcb10a2aaf5470ca8f12d6968b5e0726",
  "lastPushDate": "2024-01-24T17:55:47.000Z",
  "lastActivityDate": "2024-02-27T22:12:12.000Z",
  "mergeOfferDate": "2024-02-27T21:50:51.000Z",
  "mergeRequestDate": "2024-02-27T22:12:12.000Z",
  "mergeRequestUser": "malthe",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "newman",
      "kind": "edit",
      "files": [
        {
          "path": "types/newman/newman-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "LogvinovLeon"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "name": "node-red",
      "kind": "edit",
      "files": [
        {
          "path": "types/node-red/node-red-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "andersea",
        "tbowmo",
        "bernardobelchior",
        "alexk111",
        "Shaquu"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "name": "node",
      "kind": "edit",
      "files": [
        {
          "path": "types/node/events.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/node-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/node/test/events.ts",
          "kind": "test"
        },
        {
          "path": "types/node/test/events_generic.ts",
          "kind": "test"
        },
        {
          "path": "types/node/ts4.8/events.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/ts4.8/node-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/node/ts4.8/test/events.ts",
          "kind": "test"
        },
        {
          "path": "types/node/ts4.8/test/events_generic.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v16/events.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v16/node-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v16/test/events.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v16/test/events_generic.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v16/ts4.8/events.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v16/ts4.8/node-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v16/ts4.8/test/events.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v16/ts4.8/test/events_generic.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v18/events.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v18/node-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v18/test/events.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v18/test/events_generic.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v18/ts4.8/events.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v18/ts4.8/node-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v18/ts4.8/test/events.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v18/ts4.8/test/events_generic.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "Microsoft",
        "jkomyno",
        "alvis",
        "r3nya",
        "btoueg",
        "smac89",
        "touffy",
        "DeividasBakanas",
        "eyqs",
        "Hannes-Magnusson-CK",
        "hoo29",
        "kjin",
        "ajafff",
        "islishude",
        "mwiktorczyk",
        "mohsen1",
        "n-e",
        "galkin",
        "parambirs",
        "eps1lon",
        "ThomasdenH",
        "WilcoBakker",
        "wwwy3y3",
        "samuela",
        "kuehlein",
        "bhongy",
        "chyzwar",
        "trivikr",
        "yoursunny",
        "qwelias",
        "ExE-Boss",
        "peterblazejewicz",
        "addaleax",
        "victorperin",
        "ZYSzys",
        "NodeJS",
        "LinusU",
        "wafuwafu13",
        "mcollina",
        "Semigradsky"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    },
    {
      "name": "opossum",
      "kind": "edit",
      "files": [
        {
          "path": "types/opossum/opossum-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "quinnlangille",
        "merufm",
        "lance",
        "mastermatt",
        "tjenkinson"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Popular"
    },
    {
      "name": "rdf-store-fs",
      "kind": "edit",
      "files": [
        {
          "path": "types/rdf-store-fs/rdf-store-fs-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "tpluscode"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "name": "sse",
      "kind": "edit",
      "files": [
        {
          "path": "types/sse/sse-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "yutak23"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "name": "twitter",
      "kind": "edit",
      "files": [
        {
          "path": "types/twitter/twitter-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "BendingBender"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "name": "xml-flow",
      "kind": "edit",
      "files": [
        {
          "path": "types/xml-flow/xml-flow-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "Warerebel"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "jakebailey",
      "date": "2024-02-27T21:50:05.000Z",
      "isMaintainer": true
    },
    {
      "type": "stale",
      "reviewer": "yoursunny",
      "date": "2024-02-11T19:12:22.000Z",
      "abbrOid": "ab0ed35"
    },
    {
      "type": "stale",
      "reviewer": "mcollina",
      "date": "2024-01-26T18:08:21.000Z",
      "abbrOid": "430b6d9"
    }
  ],
  "mainBotCommentID": 1908647334,
  "ciResult": "pass"
}

typescript-bot · 2024-01-24T17:56:30Z

🔔 @Seikho @jonnysparkplugs @microsoft @jkomyno @alvis @r3nya @btoueg @smac89 @Touffy @DeividasBakanas @eyqs @Hannes-Magnusson-CK @hoo29 @kjin @ajafff @islishude @mwiktorczyk @mohsen1 @n-e @galkin @parambirs @eps1lon @ThomasdenH @WilcoBakker @wwwy3y3 @samuela @kuehlein @bhongy @chyzwar @trivikr @yoursunny @qwelias @ExE-Boss @peterblazejewicz @addaleax @victorperin @ZYSzys @nodejs @LinusU @wafuwafu13 @mcollina @Semigradsky — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

typescript-bot · 2024-01-24T19:35:58Z

@malthe The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

Semigradsky · 2024-01-24T19:48:13Z

Nice try

typescript-bot · 2024-01-25T01:03:42Z

@malthe The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

typescript-bot · 2024-01-25T10:28:48Z

⚠️ There are too many reviewers for this PR change (55). Merging can only be handled by a DT maintainer.

People who would have been pinged

LogvinovLeon andersea tbowmo bernardobelchior alexk111 Shaquu Microsoft jkomyno alvis r3nya btoueg smac89 touffy DeividasBakanas eyqs Hannes-Magnusson-CK hoo29 kjin ajafff islishude mwiktorczyk mohsen1 n-e galkin parambirs eps1lon ThomasdenH WilcoBakker wwwy3y3 samuela kuehlein bhongy chyzwar trivikr yoursunny qwelias ExE-Boss peterblazejewicz addaleax victorperin ZYSzys NodeJS LinusU wafuwafu13 mcollina Semigradsky quinnlangille merufm lance mastermatt tjenkinson tpluscode yutak23 BendingBender Warerebel

malthe · 2024-01-25T10:41:57Z

A note on the design presented here:

There are some technical issues around Function vs (...args: any[]) => void. To allow existing tests to run unchanged, I have had to differentiate between these two and right now calling these types Listener1 and Listener2. Not ideal naming.
There are some test failures that are trivial to fix but annoying:
```
6>   27:5  error  [email protected], 4.8, 4.9, 5.0, 5.1, 5.2, 5.3, 5.4 expected type to be:
6>   EventEmitter
6> got:
6>   EventEmitter<DefaultEventMap>  @definitelytyped/expect
```
These cases can mostly be identified using:
```
$ grep -R --exclude-dir=node_modules '// $ExpectType EventEmitter'
```
See Generics with default and $ExpectType value microsoft/DefinitelyTyped-tools#919.

Some packages such as @types/artillery do have an EventEmitter type that's tested using "$ExpectType" but defined as an interface:
```
export interface EventEmitter extends events.EventEmitter {}
```
In this case, the default generic type parameters do not appear in the listed type.

mcollina

LGTM, clever trick.

typescript-bot · 2024-01-26T23:05:16Z

@mcollina Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

jakebailey · 2024-02-26T23:07:55Z

Thanks for updating the PR; are there any remaining concerns? This PR had a couple previous thumbs up, and I don't have any particular notes about this besides that it seems pretty complicated for just a mapping. (I think updating the actual mechanism for consuming T would still be possible post-merge.)

But, if it seems fine, we can try and revert if it breaks.

malthe · 2024-02-27T06:25:56Z

@jakebailey definitely agree that the complexity in the implementation is considerable (and somehow not particularly elegant), but I think the upshot is that the tests demonstrate that you can use this new generic typing in a simple, straight-forward way. I think we can iterate further on the implementation post-merge.

jakebailey

Okay, then I guess we'll give it a try.

malthe · 2024-02-27T22:12:12Z

Ready to merge

ckohen · 2024-06-24T09:52:38Z

Is there a reason generics were not added to the static methods? It is totally possible to detect the event map even in the static methods and provide the same safety there. I would be happy to do so but wanted to confirm there wasn't a reason it wasn't done in the first place (couldn't see one in reading through the PR).

malthe · 2024-06-25T05:21:30Z

@ckohen no reason as far as I am aware.

Fixed the type of `fs.watch`, which may pass to `null` for the callback (DefinitelyTyped/DefinitelyTyped#65505). Removed `WritableStream` type, since its events don't match `busboy` anymore (DefinitelyTyped/DefinitelyTyped#68313).

malthe requested review from eps1lon and peterblazejewicz as code owners January 24, 2024 17:55

typescript-bot added the Critical package label Jan 24, 2024

typescript-bot added the The CI failed When GH Actions fails label Jan 24, 2024

malthe force-pushed the generic-events branch from d8dac2d to 109c2e9 Compare January 24, 2024 23:11

typescript-bot added Edits multiple packages and removed The CI failed When GH Actions fails labels Jan 24, 2024

malthe force-pushed the generic-events branch from 109c2e9 to 155a6df Compare January 24, 2024 23:23

typescript-bot added the The CI failed When GH Actions fails label Jan 25, 2024

malthe force-pushed the generic-events branch from 155a6df to 3c7d3fc Compare January 25, 2024 10:27

malthe requested a review from borisyankov as a code owner January 25, 2024 10:27

typescript-bot added Too Many Owners and removed The CI failed When GH Actions fails labels Jan 25, 2024

malthe marked this pull request as draft January 25, 2024 10:38

malthe force-pushed the generic-events branch from 3c7d3fc to b0efe22 Compare January 26, 2024 11:47

malthe changed the title ~~[node] Add generic type to 'EventEmitter'~~ [node] Add generics to 'EventEmitter' Jan 26, 2024

malthe force-pushed the generic-events branch 2 times, most recently from acd8950 to 430b6d9 Compare January 26, 2024 16:22

mcollina approved these changes Jan 26, 2024

View reviewed changes

malthe force-pushed the generic-events branch from 430b6d9 to 87bc265 Compare January 26, 2024 21:24

malthe marked this pull request as ready for review January 26, 2024 21:24

malthe added 2 commits February 26, 2024 22:21

Add additional tests

c76a55c

Workaround for case with a single key in the record type

42c56de

malthe force-pushed the generic-events branch from a2161fe to c0da831 Compare February 26, 2024 21:24

typescript-bot added Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. and removed Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. labels Feb 26, 2024

malthe force-pushed the generic-events branch from f4d45ea to bc3a6b9 Compare February 26, 2024 21:47

malthe added 2 commits February 26, 2024 23:06

Add generic type to 'EventEmitter' (v18)

9a0425b

Add generic type to 'EventEmitter' (v16)

8838f07

malthe force-pushed the generic-events branch from bc3a6b9 to 8838f07 Compare February 26, 2024 22:06

jakebailey approved these changes Feb 27, 2024

View reviewed changes

typescript-bot added Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner and removed Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. labels Feb 27, 2024

typescript-bot merged commit 2a3d411 into DefinitelyTyped:master Feb 27, 2024

This was referenced Feb 28, 2024

[node] 'EventEmitter' generics cleanup #68798

Merged

Use 'EventEmitter' with generics malthe/ts-postgres#105

Merged

jakebailey mentioned this pull request Feb 29, 2024

feat(node): add typesafety to EventEmitter events via generics (proposal) #44811

Closed

10 tasks

malthe mentioned this pull request Mar 9, 2024

Narrowing with conditional types microsoft/TypeScript#57707

Closed

mxschmitt mentioned this pull request Apr 11, 2024

[Bug]: EventEmitter types in electron.d.ts colliding with Node JS types. electron/electron#40942

Closed

3 tasks

This was referenced Jun 24, 2024

Suggestion: Reopen static and instance side of classes microsoft/TypeScript#2957

Open

types(Client): EventEmitter static method overrides discordjs/discord.js#10360

Merged

malthe deleted the generic-events branch June 25, 2024 05:16

ckohen mentioned this pull request Jul 8, 2024

[node] Add generics to static members of EventEmitter #69997

Merged

8 tasks

Julusian mentioned this pull request Jul 18, 2024

feat: add types to browser eventemitter onlxltd/bonjour-service#56

Open

[node] Add generics to 'EventEmitter' #68313

[node] Add generics to 'EventEmitter' #68313

Uh oh!

Conversation

malthe commented Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

malthe commented Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

8 packages in this PR

Code Reviews

Status

Uh oh!

typescript-bot commented Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Jan 24, 2024

Uh oh!

Semigradsky commented Jan 24, 2024

Uh oh!

typescript-bot commented Jan 25, 2024

Uh oh!

typescript-bot commented Jan 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

malthe commented Jan 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Jan 26, 2024

Uh oh!

jakebailey commented Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

malthe commented Feb 27, 2024

Uh oh!

jakebailey left a comment

Choose a reason for hiding this comment

Uh oh!

malthe commented Feb 27, 2024

Uh oh!

ckohen commented Jun 24, 2024

Uh oh!

malthe commented Jun 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

malthe commented Jan 24, 2024 •

edited

Loading

malthe commented Jan 24, 2024 •

edited

Loading

typescript-bot commented Jan 24, 2024 •

edited

Loading

typescript-bot commented Jan 24, 2024 •

edited

Loading

typescript-bot commented Jan 25, 2024 •

edited

Loading

malthe commented Jan 25, 2024 •

edited

Loading

jakebailey commented Feb 26, 2024 •

edited

Loading