[node] events: refactor module structure#70635
[node] events: refactor module structure#70635Renegade334 wants to merge 3 commits intoDefinitelyTyped:masterfrom
Conversation
9f6031b to
fc74ffa
Compare
|
@Renegade334 Thank you for submitting this PR! This is a live comment that I will keep updated. 1 package in this PRCode 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
Once every item on this list is checked, I'll ask you for permission to merge and publish the changes. InactiveThis PR has been inactive for 10 days. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 70635,
"author": "Renegade334",
"headCommitOid": "a293e4fa6171cd41843c31cc39c2b064017fc997",
"mergeBaseOid": "2fc2fb9ba5cc0b68fe14c9fd9e21fadbd9a781a6",
"lastPushDate": "2024-09-22T11:53:23.000Z",
"lastActivityDate": "2024-09-24T22:06:15.000Z",
"hasMergeConflict": false,
"isFirstContribution": false,
"tooManyFiles": false,
"hugeChange": false,
"popularityLevel": "Critical",
"pkgInfo": [
{
"name": "node",
"kind": "edit",
"files": [
{
"path": "types/node/events.d.ts",
"kind": "definition"
},
{
"path": "types/node/test/events.ts",
"kind": "test"
},
{
"path": "types/node/v18/events.d.ts",
"kind": "definition"
},
{
"path": "types/node/v18/test/events.ts",
"kind": "test"
},
{
"path": "types/node/v20/events.d.ts",
"kind": "definition"
},
{
"path": "types/node/v20/test/events.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"
}
],
"reviews": [],
"mainBotCommentID": 2366820581,
"ciResult": "fail",
"ciUrl": "https://github.com/DefinitelyTyped/DefinitelyTyped/commit/a293e4fa6171cd41843c31cc39c2b064017fc997/checks?check_suite_id=29238045633"
} |
|
🔔 @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 |
fc74ffa to
8c04086
Compare
|
Are you skipping the refactor in the v16 dir intentionally? (I know it's technically EOL, though we're fiddling with things for TS changes and v16 still gets a good 800k weekly downloads.) Is it accurate to say that this should go in before #69997 (if we're taking that)? |
I was, but that's not to say that it can't be included.
The two don't really overlap in their scope, although if the problematic efforts to genericise the top-level |
|
Given we're still noodling on fixing iterator/buffer related things that are getting patched in v16, I think it would probably be worthwhile to apply this fix there too for the time being to keep everything consistent; hopefully we can drop that at some point. Sorry to make more work :( |
Worth noting this is exactly the reason that pr was open in the first place. reopening class definitions with a namespace (the only way to modify static members) was possible due to a ts bug, and fixing that broke this very desirable behavior. I still believe having native overloads is extremely important, but this lessens the need. I think realistically these want to land around the same time if possible, the change in the |
|
@Renegade334 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! |
8c04086 to
a293e4f
Compare
|
Re-ping @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: This PR has been out for over a week, yet I haven't seen any reviews. Could someone please give it some attention? Thanks! |
|
@Renegade334 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. |
|
Blocking on #69997. |
|
I thought this would land first, which is why I hadn't rebased yet. I can work with either way. |
|
Think it'd be better to merge the event map changes first. |
The structure of the events module in @types/node hasn't really changed since TS3, and is an unholy assortment of definition paradigms and circuitous import-export behaviour (the module imports itself, via a different module, in order to re-declare itself as its own namespace property...) Additionally, the top-level functions/variables in the events module are currently defined as static properties on the EventEmitter "class", which doesn't mirror how they're implemented or documented in Node, and which subjects them to unnecessary accessibility checks etc. by the compiler.
This changeset looks large, but it's only a small number of restructuring tasks:
There should be no change in import behaviour: both CJS- and ESM-style imports will continue to work the same as before.
This also includes an additional change in v22, which is to move the EventEmitter class definitions from the
NodeJS.EventEmitterinterface to the EventEmitter class itself.NodeJS.EventEmitterwas the global event emitter interface added to @types/node in v0, prior to the existence of the events module and the EventEmitter class, which hangs around now as a vestigial entity that simply doesn't need to exist any more. While this change is transparent to consumers of the EventEmitter class, and keeps the global interface unaltered, it lays the groundwork for removingNodeJS.EventEmitterentirely in the next major @types/node version.