[node] Add generics to static members of EventEmitter#69997
[node] Add generics to static members of EventEmitter#69997typescript-bot merged 4 commits intoDefinitelyTyped:masterfrom
EventEmitter#69997Conversation
|
@ckohen Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through. This is a live comment that I will keep updated. 14 packages in this PR
Code ReviewsBecause 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
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": 69997,
"author": "ckohen",
"headCommitOid": "879828fe48aa6ba3504e155599ef131cb868cd7f",
"mergeBaseOid": "8b50e02d218372520eb4727865fc9ae799c789a1",
"lastPushDate": "2024-07-08T01:23:57.000Z",
"lastActivityDate": "2024-10-31T05:12:37.000Z",
"mergeOfferDate": "2024-10-28T18:38:05.000Z",
"mergeRequestDate": "2024-10-31T05:12:37.000Z",
"mergeRequestUser": "ckohen",
"hasMergeConflict": false,
"isFirstContribution": true,
"tooManyFiles": false,
"hugeChange": false,
"popularityLevel": "Critical",
"pkgInfo": [
{
"name": "imap",
"kind": "edit",
"files": [
{
"path": "types/imap/index.d.ts",
"kind": "definition"
}
],
"owners": [
"psnider"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Popular"
},
{
"name": "jake",
"kind": "edit",
"files": [
{
"path": "types/jake/index.d.ts",
"kind": "definition"
}
],
"owners": [],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Well-liked by everyone"
},
{
"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/test/events.ts",
"kind": "test"
},
{
"path": "types/node/test/events_generic.ts",
"kind": "test"
},
{
"path": "types/node/v16/events.d.ts",
"kind": "definition"
},
{
"path": "types/node/v16/test/events_generic.ts",
"kind": "test"
},
{
"path": "types/node/v18/events.d.ts",
"kind": "definition"
},
{
"path": "types/node/v18/test/events.ts",
"kind": "test"
},
{
"path": "types/node/v18/test/events_generic.ts",
"kind": "test"
},
{
"path": "types/node/v20/events.d.ts",
"kind": "definition"
},
{
"path": "types/node/v20/test/events.ts",
"kind": "test"
},
{
"path": "types/node/v20/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",
"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": "readable-stream",
"kind": "edit",
"files": [
{
"path": "types/readable-stream/index.d.ts",
"kind": "definition"
}
],
"owners": [
"TeamworkGuy2",
"markdreyer",
"mcollina"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Critical"
},
{
"name": "sane",
"kind": "edit",
"files": [
{
"path": "types/sane/index.d.ts",
"kind": "definition"
}
],
"owners": [
"BendingBender"
],
"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": "steam",
"kind": "edit",
"files": [
{
"path": "types/steam/index.d.ts",
"kind": "definition"
}
],
"owners": [
"kant2002"
],
"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": "umzug",
"kind": "edit",
"files": [
{
"path": "types/umzug/index.d.ts",
"kind": "definition"
}
],
"owners": [
"drinchev",
"mlamp",
"trodi",
"emmanuelgautier"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Popular"
},
{
"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": "gabritto",
"date": "2024-10-28T18:37:17.000Z",
"isMaintainer": true
},
{
"type": "stale",
"reviewer": "Harry5012",
"date": "2024-08-28T05:55:24.000Z",
"abbrOid": "ce5426d"
},
{
"type": "stale",
"reviewer": "sheetalkamat",
"date": "2024-08-06T22:14:48.000Z",
"abbrOid": "ae1dd74"
},
{
"type": "stale",
"reviewer": "Renegade334",
"date": "2024-08-04T22:32:38.000Z",
"abbrOid": "ae1dd74"
},
{
"type": "stale",
"reviewer": "Semigradsky",
"date": "2024-07-30T19:58:25.000Z",
"abbrOid": "dce179a"
},
{
"type": "stale",
"reviewer": "quinnlangille",
"date": "2024-07-23T15:27:38.000Z",
"abbrOid": "28aa21c"
},
{
"type": "stale",
"reviewer": "Anonymous4078",
"date": "2024-07-17T03:08:35.000Z",
"abbrOid": "c05cc61"
}
],
"mainBotCommentID": 2212743501,
"ciResult": "pass"
} |
|
🔔 @microsoft @jkomyno @alvis @r3nya @btoueg @smac89 @Touffy @DeividasBakanas @eyqs @Hannes-Magnusson-CK @hoo29 @kjin @ajafff @islishude @mwiktorczyk @mohsen1 @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 |
|
@ckohen 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 that are failing do not end up on the list of PRs for the DT maintainers to review. |
468eea3 to
118c57c
Compare
People who would have been pingedpsnider LogvinovLeon andersea tbowmo bernardobelchior alexk111 Shaquu Microsoft jkomyno alvis r3nya btoueg smac89 touffy DeividasBakanas eyqs Hannes-Magnusson-CK hoo29 kjin ajafff islishude mwiktorczyk mohsen1 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 TeamworkGuy2 markdreyer BendingBender yutak23 kant2002 drinchev mlamp trodi emmanuelgautier Warerebel |
|
@ckohen 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 that are failing do not end up on the list of PRs for the DT maintainers to review. |
118c57c to
c05cc61
Compare
|
@ckohen 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 that are failing do not end up on the list of PRs for the DT maintainers to review. |
|
As far as I can tell, everything that fails in CI is either unaffected by the changes I made or should've never passed in the first place. Basically every quick way to generate types for some of the methods (before the original PR) narrows the parameters of said methods, which is not allowed because it breaks assignability...as CI correctly identifies. |
|
@ckohen One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you! |
c05cc61 to
d932023
Compare
As mentioned in my previous comment, the CI is failing correctly. I need some guidance from maintainers because those tests should have never ever passed in the first place. |
|
@ckohen 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 that are failing do not end up on the list of PRs for the DT maintainers to review. |
|
CI has to pass for this to go in, yes; the other packages have to be updated to pass. This also may be an indicator that this sort of change would break packages outside of DT as well. |
d932023 to
01e0133
Compare
|
@ckohen Unfortunately, this pull request currently has a merge conflict 😥. Please update your PR branch to be up-to-date with respect to master. Have a nice day! |
Fixes bugs in / improves generics of non-static members
* fix: EE.on/once inference with extended EventEmitter classes * chore: undo bad copy paste * fix: node 18 types, node 16 soon * chore: make brand optional * chore: fix node 16 * chore: lint * chore: remove useless definition of brand * chore: requested changes
366e882 to
879828f
Compare
|
Ready to merge |
…ventEmit…" This reverts commit 1d35480.
This change, DefinitelyTyped/DefinitelyTyped#69997, to the `@types/node` package made typings for the emitter stricter and break the builds. Functionally the code is the same.
|
Oh boy... I'd argue that people who use node types that point to node 18+ shouldn't be trying to compile to ES < 2015 anyways but that aside... would the use of a hidden symbol to achieve the same result work better for compatibility? |
|
I also experienced problems unrelated to es5. The change of the types of Effectively became: Which is maybe more correct, but broke many things for me. Resulting in: For anything that proxies these return values. |
|
That's gonna end up asking the question: are we ok with breaking some old code to get stricter, more accurate functions or keep requiring casts or third party packages to achieve the same features... (That said, those I'm gonna take a wild shot and ask: what should we do here? We can revert the revert and move from private field to symbol property (and ensure that doesn't break weird quirks like |
|
It is a little interesting because in a more conventional package you would make that break on a major version and it wouldn't be a big deal. That isn't really a possibility here, so it just comes as a bit of a surprise. I do think that breaking the types to make them more accurate is reasonable (if done sparingly). In the case I have we have some implementation that we make have a similar external interface to an old implementation. A class that inherits from a base class (that isn't node specific and has no emitter) and the derived class is node specific, and then use a mixin-like approach to make it an emitter. Which proxies things to an emitter member. The signatures all match the signatures in the previous package. Things can be re-worked to have an intermediate base class that extends emitter and then the leaf class extends from that instead. That would not break from changes such as this. (Also should constrain the types, but it was emulating an existing interface.) Though someone using the return type in specific ways may still have a bad time. |
The thing with "accuracy" with regard to event maps here is that they don't arise from the Node API itself; it describes user events as arbitrary, with the "official" event parameter type being Extending this non-essential feature beyond the I can't help but feel that the simplest solution is best here: leave those two functions as non-generic and broadly typed, make any last tweaks to the EventEmitter method definitions to get them playing nicely, and live to fight another day... |
|
The NoInfer option is honestly the best if we're not using a vanity property (with maybe a fix for type param inference at some point in the future). However if we are forced to support es2015 target (which is only mostly absurd) I can't foresee any support for ts 5.4 as a minimum version. The breaks we are seeing suggest improper tsconfigs which is definitely a very prevalent problem. I have seen many tsconfigs that make absolutely 0 sense for the project but is probably just copied from some template that was made years ago. This is a completely separate issue, but it's been highlighted here. |
While I originally set out to just add generics to static members of
EventEmitter, I discovered a few quirks and bugs in the existing generic code that made it very hard to do well. In order to make the static members work I had to fix these bugs, which ends up being possibly a little clearer to understand.The original PR used T, K, etc... for type param ids, which I have expanded in my cleanup process to better help understand what said generics did. I think this would be helpful for everyone, so I've left it, but wasn't sure if it was a stylistic choice.
The biggest change worth noting here (of the bug fixes) is that the types will now still allow you to emit, listen, etc... arbitrary events. It maintains type safety for events you specify as having strict types but prefers to more strictly match the runtime when straying outside those events.
The change from
Functionto a function signature is the only slight breaking change. It notably brought to light some packages that were overriding the built-in types ofEventEmitterfrom the days when they were still implementing the interface instead of extending the class.Please fill in this template.
pnpm test <package to test>.If changing an existing definition:
package.json.