node: Backport #59905 to ts4.8 #62629

meyfa · 2022-10-09T10:30:57Z

Node.js types are currently out of sync between TS 4.9+ and TS <= 4.8 variants. Specifically, PR #59905 introduced many changes that were not copied over to ts4.8. This patch is an exact copy of that PR, but applied to ts4.8. The goal is that @types/node behaves the same on all versions of TypeScript.

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 npm test <package to test>.

If changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes:
- See [node] remove lib: "dom"; add global Event and EventTarget, fix other global types #59905 for the source of this patch (compare the diff to this PR via the "Files changed" tab - they should be identical).
- The ts4.8 directories were introduced in Update node types for TS 4.9 beta #62375.

Node.js types are currently out of sync between TS 4.9+ and TS <= 4.8 variants. Specifically, PR #59905 introduced many changes that were not copied over to ts4.8. This patch is an exact copy of that PR, but applied to ts4.8. The goal is that `@types/node` behaves the same on all versions of TypeScript.

typescript-bot · 2022-10-09T10:31:38Z

@meyfa Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

node — on npm, on unpkg
- owner-approval: @peterblazejewicz
- Config files to check:
  - node/ts4.8/tsconfig.json: not the expected form and not moving towards it (check: compilerOptions.target)

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 module config files

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": 62629,
  "author": "meyfa",
  "headCommitOid": "e13e1b9c275dfc02fb37c7f346b33714dcd42a5b",
  "mergeBaseOid": "29fc0c582c5d9dc2fd41b4c9ca576ea40ccd4d15",
  "lastPushDate": "2022-10-09T10:24:50.000Z",
  "lastActivityDate": "2022-10-10T21:10:56.000Z",
  "mergeOfferDate": "2022-10-10T19:42:24.000Z",
  "mergeRequestDate": "2022-10-10T21:10:56.000Z",
  "mergeRequestUser": "meyfa",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "node",
      "kind": "edit",
      "files": [
        {
          "path": "types/node/ts4.8/buffer.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/ts4.8/dom-events.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/ts4.8/events.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/ts4.8/globals.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/ts4.8/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/ts4.8/perf_hooks.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/ts4.8/stream.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/ts4.8/stream/consumers.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/ts4.8/test/buffer.ts",
          "kind": "test"
        },
        {
          "path": "types/node/ts4.8/test/crypto.ts",
          "kind": "test"
        },
        {
          "path": "types/node/ts4.8/test/events.ts",
          "kind": "test"
        },
        {
          "path": "types/node/ts4.8/test/perf_hooks.ts",
          "kind": "test"
        },
        {
          "path": "types/node/ts4.8/test/stream.ts",
          "kind": "test"
        },
        {
          "path": "types/node/ts4.8/test/url.ts",
          "kind": "test"
        },
        {
          "path": "types/node/ts4.8/test/util.ts",
          "kind": "test"
        },
        {
          "path": "types/node/ts4.8/test/wasi.ts",
          "kind": "test"
        },
        {
          "path": "types/node/ts4.8/test/worker_threads.ts",
          "kind": "test"
        },
        {
          "path": "types/node/ts4.8/tsconfig.json",
          "kind": "package-meta",
          "suspect": "not [the expected form](https://github.com/DefinitelyTyped/DefinitelyTyped#user-content-tsconfigjson) and not moving towards it (check: `compilerOptions.target`)"
        },
        {
          "path": "types/node/ts4.8/url.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/ts4.8/util.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/ts4.8/worker_threads.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v16/ts4.8/buffer.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v16/ts4.8/stream/consumers.d.ts",
          "kind": "definition"
        }
      ],
      "owners": [
        "Microsoft",
        "DefinitelyTyped",
        "jkomyno",
        "alvis",
        "r3nya",
        "btoueg",
        "smac89",
        "touffy",
        "DeividasBakanas",
        "eyqs",
        "Hannes-Magnusson-CK",
        "hoo29",
        "kjin",
        "ajafff",
        "islishude",
        "mwiktorczyk",
        "mohsen1",
        "n-e",
        "galkin",
        "parambirs",
        "eps1lon",
        "SimonSchick",
        "ThomasdenH",
        "WilcoBakker",
        "wwwy3y3",
        "samuela",
        "kuehlein",
        "bhongy",
        "chyzwar",
        "trivikr",
        "yoursunny",
        "qwelias",
        "ExE-Boss",
        "peterblazejewicz",
        "addaleax",
        "victorperin",
        "ZYSzys",
        "NodeJS",
        "LinusU",
        "wafuwafu13",
        "mcollina"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "peterblazejewicz",
      "date": "2022-10-10T19:41:39.000Z",
      "isMaintainer": true
    }
  ],
  "mainBotCommentID": 1272511058,
  "ciResult": "pass"
}

typescript-bot · 2022-10-09T10:31:41Z

🔔 @microsoft @DefinitelyTyped @jkomyno @alvis @r3nya @btoueg @smac89 @Touffy @DeividasBakanas @eyqs @Hannes-Magnusson-CK @hoo29 @kjin @ajafff @islishude @mwiktorczyk @mohsen1 @n-e @galkin @parambirs @eps1lon @SimonSchick @ThomasdenH @WilcoBakker @wwwy3y3 @samuela @kuehlein @bhongy @chyzwar @trivikr @yoursunny @qwelias @ExE-Boss @peterblazejewicz @addaleax @victorperin @ZYSzys @nodejs @LinusU @wafuwafu13 @mcollina — 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.

meyfa · 2022-10-09T10:32:13Z

CC @thw0rted are you okay with this change? Thanks for the great work on the original patch, by the way! Very awesome! :)

thw0rted · 2022-10-09T11:14:16Z

Thanks, I'm glad it's working out. I just skimmed the diff but it looks good, as long as all the tests are passing and whatnot.

peterblazejewicz

@meyfa thanks!

meyfa · 2022-10-10T21:10:56Z

Ready to merge

AviVahl · 2022-10-10T23:00:35Z

v16.11.65 caused a regression on my end. Can be seen in https://github.com/wixplosives/pleb, by cloning, removing the lock file, and running npm it:

node_modules/@types/node/ts4.8/stream.d.ts:842:39 - error TS2304: Cannot find name 'Blob'.

842             static from(src: Stream | Blob | ArrayBuffer | string | Iterable<any> | AsyncIterable<any> | AsyncGeneratorFunction | Promise<any> | Object): Duplex;
                                          ~~~~

node_modules/@types/node/ts4.8/stream.d.ts:842:39 - error TS2304: Cannot find name 'Blob'.

842             static from(src: Stream | Blob | ArrayBuffer | string | Iterable<any> | AsyncIterable<any> | AsyncGeneratorFunction | Promise<any> | Object): Duplex;

Can be worked around by adding "dom" to tsconfig->lib, which kinda beats the purpose of this PR.

thw0rted · 2022-10-11T09:23:51Z

I think I see the issue. In the updated tests, we check for new-able global Blob but not the type side (const b: Blob). Conditional types were used to declare global interfaces for e.g. Event / EventType but not Blob, because Blob is a class but Event is not.

I put together a quick PR that will add the type-side of Blob (along with tests): #62654

DefinitelyTyped/DefinitelyTyped#62629 (comment)

OpportunityLiu · 2022-10-12T07:57:10Z

After upgrade to @types/node@18.8.4, window becomes a constructor.

OpportunityLiu · 2022-10-12T08:00:41Z

types/node/ts4.8/dom-events.d.ts

+
+// See comment above explaining conditional type
+type __EventTarget = typeof globalThis extends { onmessage: any, EventTarget: infer T }
+? T


Maybe T['prototype'] ?

thw0rted · 2022-10-12T13:00:15Z

The spec for EventTarget says it has a constructor, so the type declaration for EventTarget is new-able. Window was already new-able before this PR. I'm not sure I understand the issue here?

OpportunityLiu · 2022-10-12T13:58:25Z

window is not a function, Window is. After this PR, call new window() got an EventTarget

OpportunityLiu · 2022-10-12T14:02:07Z

And for Event & EventTarget, I don't think you can new them repeatly.
https://www.typescriptlang.org/play?#code/MYewdgzgLgBA7jAvDMBTBcCWYAmI4DcAUKJLKrMmggBTUx3owCiAbqmFACoCGATgHMKASmHFS0GKlZIUTRrXpsOUGgHJUa0es3aNWgkA

https://www.typescriptlang.org/play?&dtPR=62629&install-plugin=playground-dt-review#code/JYWwDg9gTgLgBAbwL5wGZQiOAiAdhAEwFNsAoUgYwlwGd4B3OAXjlyMfuFwInoG5K1OnCLwWbRgAoJcaezgBRAG5FcMACoBDKAHNRASn2Da8Ikuat5cqTOWqYkgORFHhpy7fPXQA

thw0rted · 2022-10-12T17:03:19Z

OK, I think I'm going to need someone smarter than I am to explain why changing ? T to ? T['prototype'] fixes this, but it seems like probably it does? As you point out, the current syntax (? T : ...) makes instances of EventTarget (and all the other conditional types like this) recursively new-able, in a lib-dom environment. I have no problem with making that change where appropriate, but even better would be to have some way of testing this behavior with dtslint. I have no idea how to do that; I'm not sure if it's possible to make dtslint run against multiple tsconfig files.

thw0rted · 2022-10-17T15:43:56Z

Is anybody still looking at this? Is there maybe a new issue? I'm having a really hard time putting together a reliable reproduction. I just want to add tests that say /** @ts-expect-error */ new (new EventTarget())() or something along those lines, for each interface that uses this conditional-type pattern, then have the test fail, then throw in the ['prototype]' and watch the red lights go green. I haven't been able to get it working yet, unfortunately.

DefinitelyTyped/DefinitelyTyped#62629 (comment)

meyfa requested review from eps1lon and peterblazejewicz as code owners October 9, 2022 10:30

typescript-bot added Critical package Check Config Changes a module config files labels Oct 9, 2022

peterblazejewicz approved these changes Oct 10, 2022

View reviewed changes

typescript-bot added Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner labels Oct 10, 2022

typescript-bot merged commit 85092f9 into DefinitelyTyped:master Oct 10, 2022

72636c mentioned this pull request Oct 11, 2022

Disable major type-fest renovation seek-oss/skuba#990

Merged

1 task

iCrawl added a commit to discordjs/discord.js that referenced this pull request Oct 11, 2022

fix: pin @types/node version

9d8179c

DefinitelyTyped/DefinitelyTyped#62629 (comment)

OpportunityLiu reviewed Oct 12, 2022

View reviewed changes

meyfa deleted the node/ts4.8-59905 branch October 12, 2022 12:30

sandersn mentioned this pull request Oct 18, 2022

Node global interfaces redux #62782

Merged

AdaptingAFM pushed a commit to AdaptingAFM/ui that referenced this pull request Oct 5, 2024

fix: pin @types/node version

96bf53c

DefinitelyTyped/DefinitelyTyped#62629 (comment)

node: Backport #59905 to ts4.8 #62629

node: Backport #59905 to ts4.8 #62629

Uh oh!

Conversation

meyfa commented Oct 9, 2022

Uh oh!

typescript-bot commented Oct 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1 package in this PR

Code Reviews

Status

Uh oh!

typescript-bot commented Oct 9, 2022

Uh oh!

meyfa commented Oct 9, 2022

Uh oh!

thw0rted commented Oct 9, 2022

Uh oh!

peterblazejewicz left a comment

Choose a reason for hiding this comment

Uh oh!

meyfa commented Oct 10, 2022

Uh oh!

AviVahl commented Oct 10, 2022

Uh oh!

thw0rted commented Oct 11, 2022

Uh oh!

OpportunityLiu commented Oct 12, 2022

Uh oh!

OpportunityLiu Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

thw0rted commented Oct 12, 2022

Uh oh!

OpportunityLiu commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OpportunityLiu commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thw0rted commented Oct 12, 2022

Uh oh!

thw0rted commented Oct 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

typescript-bot commented Oct 9, 2022 •

edited

Loading

OpportunityLiu commented Oct 12, 2022 •

edited

Loading

OpportunityLiu commented Oct 12, 2022 •

edited

Loading