@chwoerz Thank you for submitting this PR!

This is a live comment that I will keep updated.

2 packages in this PR

• graphql-upload — on npm, on unpkg
• node — 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 that 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.

{
  "type": "info",
  "now": "-",
  "pr_number": 71679,
  "author": "chwoerz",
  "headCommitOid": "31efc2954b34ca64cf7e53e8f569197d6dfbaa7b",
  "mergeBaseOid": "42bb005b680604130d4ae72b949d9fdbe8935fe1",
  "lastPushDate": "2025-01-17T15:53:35.000Z",
  "lastActivityDate": "2025-01-23T17:50:29.000Z",
  "mergeOfferDate": "2025-01-23T16:47:41.000Z",
  "mergeRequestDate": "2025-01-23T17:50:29.000Z",
  "mergeRequestUser": "chwoerz",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "graphql-upload",
      "kind": "edit",
      "files": [
        {
          "path": "types/graphql-upload/graphql-upload-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "mike-marcacci",
        "rdsfj"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Popular"
    },
    {
      "name": "node",
      "kind": "edit",
      "files": [
        {
          "path": "types/node/fs.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/test/fs.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",
        "NodeJS",
        "LinusU",
        "wafuwafu13",
        "mcollina",
        "Semigradsky"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "rbuckton",
      "date": "2025-01-23T16:47:03.000Z",
      "isMaintainer": true
    }
  ],
  "mainBotCommentID": 2598662845,
  "ciResult": "pass"
}

🔔 @mike-marcacci @rdsfj @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 @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.

@chwoerz 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.

Thoughts:

• My impression on why internal EventEmitters had kept explicit event overloads rather than "genericising" was to avoid causing any side-effects with assignability or extensibility, although I don't think this was ever put to the test.
• @types/node already has a feature for event maps in EventEmitters. This isn't stable enough to use internally yet, but ultimately if we're going to be going down the line of using event maps for API classes, then waiting until something like this is possible:

  export class Streamy extends stream.Readable { ... }
  export interface Streamy extends EventEmitter<StreamyEvents> {}

  probably makes more sense than adding a second parallel event map paradigm to @types/node and adding a load of additional helper types just for one individual case. As you say, this isn't adding functionality to the API but is just designed to make the definitions more readable, so there is no particular urgency.

I see your point. However the change was added already to v20 of the types so in my opinion it should also be added here.

In the future, if the other map is stable, we can then replace it easy. But in my opinion the current implementation is hard to read and its hard to understand that all events are the same for all functions.

With this change this will be made clear for the developer looking into it.

I get the argument about ease of maintenance, but given that the interface events of the FS streams have not changed since 2018, and there are no active proposals to add new ones, I'm not sure what the real benefit is to be derived here.

It's not that the objective itself is fundamentally wrong, I just don't see that causing divergence between the definitions of the API's different EventEmitters is warranted for what appears to be no tangible advantage.

As you've identified with having to modify downstream DT tests, this is also not a change with zero side-effects.

I did not mean the ease of maintenance but the ease of the dev who uses it to understand the events are the same for all the functions on first glance.

You are right that it could have some tests failing but the test which did fail in graphql-upload was an error on their side as the "finish" event does not take a parameter in the first place and the generic type of the promise needs to be set.

But lets see what the maintainers think :)

Ready to merge