Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[node] remove lib: "dom"; add global Event and EventTarget, fix other global types #59905

Merged
merged 16 commits into from Oct 3, 2022

Conversation

thw0rted
Copy link
Contributor

Please fill in this template.

If changing an existing definition:


The previous version polluted the global namespace with a Blob interface which merged (incorrectly) with the DOM Blob definition provided by ts-lib. My update uses the correct Node (experimental) Blob definition, which already existed under node:buffer.

I also took this opportunity to resolve an outstanding TODO comment, where Blob#stream() returned unknown because at the time it was originally added, the Node (experiemental) WHATWG / W3C compatible ReadableStream implementation didn't have types defined yet. They have since been created (under node:stream/web), so that method now returns the correct stream type.

@typescript-bot
Copy link
Contributor

typescript-bot commented Apr 14, 2022

@thw0rted Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

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": 59905,
  "author": "thw0rted",
  "headCommitOid": "542cacd0803f50d8537a94b2dfa3e3e125c837ac",
  "mergeBaseOid": "a6009a6e4434021ed0e67e7b74225a1190bf64c9",
  "lastPushDate": "2022-09-13T08:46:59.000Z",
  "lastActivityDate": "2022-10-03T22:01:28.000Z",
  "mergeOfferDate": "2022-10-03T21:17:54.000Z",
  "mergeRequestDate": "2022-10-03T22:01:28.000Z",
  "mergeRequestUser": "thw0rted",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "node",
      "kind": "edit",
      "files": [
        {
          "path": "types/node/buffer.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/dom-events.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/events.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/globals.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/perf_hooks.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/stream.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/stream/consumers.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/test/buffer.ts",
          "kind": "test"
        },
        {
          "path": "types/node/test/crypto.ts",
          "kind": "test"
        },
        {
          "path": "types/node/test/events.ts",
          "kind": "test"
        },
        {
          "path": "types/node/test/perf_hooks.ts",
          "kind": "test"
        },
        {
          "path": "types/node/test/stream.ts",
          "kind": "test"
        },
        {
          "path": "types/node/test/url.ts",
          "kind": "test"
        },
        {
          "path": "types/node/test/util.ts",
          "kind": "test"
        },
        {
          "path": "types/node/test/wasi.ts",
          "kind": "test"
        },
        {
          "path": "types/node/test/worker_threads.ts",
          "kind": "test"
        },
        {
          "path": "types/node/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/url.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/util.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v16/buffer.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v16/stream/consumers.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/worker_threads.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": "rbuckton",
      "date": "2022-10-03T21:17:06.000Z",
      "isMaintainer": true
    },
    {
      "type": "stale",
      "reviewer": "peterblazejewicz",
      "date": "2022-08-03T21:10:28.000Z",
      "abbrOid": "70a166f"
    },
    {
      "type": "stale",
      "reviewer": "LinusU",
      "date": "2022-07-08T12:58:43.000Z",
      "abbrOid": "00417c0"
    },
    {
      "type": "stale",
      "reviewer": "sheetalkamat",
      "date": "2022-07-05T21:33:53.000Z",
      "abbrOid": "00417c0"
    }
  ],
  "mainBotCommentID": 1099384271,
  "ciResult": "pass"
}

@typescript-bot typescript-bot added Critical package Untested Change This PR does not touch tests labels Apr 14, 2022
@typescript-bot typescript-bot added this to Waiting for Code Reviews in New Pull Request Status Board Apr 14, 2022
@thw0rted
Copy link
Contributor Author

If anybody has advice for how to test that Node types do not pollute the global namespace with a Blob interface, I'd love to add one. Failing that, behavior should remain the same as it was before.

@typescript-bot typescript-bot added the The CI failed When GH Actions fails label Apr 14, 2022
@typescript-bot
Copy link
Contributor

@thw0rted 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 typescript-bot moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board Apr 14, 2022
@typescript-bot typescript-bot added Check Config Changes a module config files and removed The CI failed When GH Actions fails Untested Change This PR does not touch tests labels Apr 15, 2022
@typescript-bot typescript-bot moved this from Needs Author Action to Waiting for Code Reviews in New Pull Request Status Board Apr 15, 2022
@typescript-bot typescript-bot added the The CI failed When GH Actions fails label Apr 15, 2022
@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board Apr 15, 2022
@typescript-bot
Copy link
Contributor

@thw0rted 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 typescript-bot added Edits multiple packages and removed The CI failed When GH Actions fails labels Apr 15, 2022
@typescript-bot typescript-bot moved this from Needs Author Action to Waiting for Code Reviews in New Pull Request Status Board Apr 15, 2022
@thw0rted
Copy link
Contributor Author

This PR is going to require a bigger change than I originally expected. The problem is that the package tsconfig is using lib: ["dom"]. I changed that to "es6", which broke a few things. As a result, I have added a full interface declaration for Node's DOM-compatibility Event and related types, under a fictitious module (node:dom-events).

If they were exported at the global level -- which, to be clear, they should be! -- they would break every project that uses both @types/node and lib.dom.d.ts, which is of course how we got into this mess in the first place. There was a long conversation about how to handle this in another issue but I don't think we really got a good answer. (Consuming code can do some trickery with conditional types evaluated against globalThis but I don't see how to fix it for use within the types-package.)

I think we may need smarter Typescripters than myself to take a look at this and suggest a way forward. Are there other fundamentally-incompatible global types that differ between Node and the DOM? If so, how have they been handled? If not... now what?

@typescript-bot typescript-bot added the The CI failed When GH Actions fails label Apr 15, 2022
@typescript-bot
Copy link
Contributor

@thw0rted 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 typescript-bot moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board Apr 15, 2022
@thw0rted
Copy link
Contributor Author

thw0rted commented Apr 15, 2022

The CI build keeps failing but I don't understand how the broken packages ever worked. Two of them depend on @types/mongodb which was removed last year (Mongo now ships their own types), and the old package is flat-out broken. One of them has a test that looks like $ExpectType "one" | "two" | "three" but that syntax never worked as far as I can tell. Any help with resolving the CI problems would be much appreciated.

@thw0rted
Copy link
Contributor Author

The change I just pushed exposes Event and EventTarget globally via the same mechanism used in #58277. I'm not sure how to test and make sure this doesn't explode real-world users...

@typescript-bot typescript-bot removed the The CI failed When GH Actions fails label Apr 15, 2022
@thw0rted
Copy link
Contributor Author

I don't want to derail this if somebody is actually planning to merge it, but at least a note for potential future work: #60924 (comment) points out that Node's fetch is actually just taken from undici (an official Node project), which ships its own types, including their interpretation of all the WHATWG streams and whatnot. Any types currently living under @types/node that also exist under undici should be removed and pulled from there instead by reference. (If Node wants to maintain their own types, that sounds great to me...)

@typescript-bot typescript-bot added the Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. label Sep 24, 2022
@typescript-bot
Copy link
Contributor

Copy link
Collaborator

@rbuckton rbuckton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor questions, but otherwise this looks fine.

types/node/stream/consumers.d.ts Show resolved Hide resolved
types/node/url.d.ts Show resolved Hide resolved
@typescript-bot 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 Oct 3, 2022
@typescript-bot typescript-bot moved this from Needs Maintainer Review to Waiting for Author to Merge in New Pull Request Status Board Oct 3, 2022
@thw0rted
Copy link
Contributor Author

thw0rted commented Oct 3, 2022

Ready to merge

@typescript-bot typescript-bot moved this from Waiting for Author to Merge to Recently Merged in New Pull Request Status Board Oct 3, 2022
@typescript-bot typescript-bot merged commit e84e5c6 into DefinitelyTyped:master Oct 3, 2022
@thw0rted thw0rted deleted the node-blob-fix branch October 3, 2022 22:02
@meyfa meyfa mentioned this pull request Oct 4, 2022
6 tasks
thw0rted added a commit to thw0rted/storybook that referenced this pull request Oct 6, 2022
After merging my [recent PR](DefinitelyTyped/DefinitelyTyped#59905) for `@types/node`, I was alerted that a couple dependents of this package were failing to compile.  I have identified a fix that I believe should preserve the original behavior of this code while resolving those errors.  I don't know anything about Storybook, so if I'm wrong, or if there's a better fix for the issue, please feel free to do your own thing.

For background: I don't think Storybook, or the packages with failing `dtslint` runs, `storybook-readme` and `storybook-addon-jsx`, directly depend on `@types/node`, but it's common for frontend packages to install build tooling that does depend on the package.  This causes Node types to be introduced to the default `typeRoot`, so you wind up with Node types merged in with those in the DOM lib.  (Storybook may be a bit of an odd duck, because it looks like in some cases, React declares its own empty DOM interfaces in `@types/react/globals.d.ts`.)

At any rate, I hope the proposed fix works for you.
thw0rted added a commit to thw0rted/storybook that referenced this pull request Oct 6, 2022
After merging my [recent PR](DefinitelyTyped/DefinitelyTyped#59905) for `@types/node`, I was alerted that a couple dependents of this package were failing to compile.  I have identified a fix that I believe should preserve the original behavior of this code while resolving those errors.  I don't know anything about Storybook, so if I'm wrong, or if there's a better fix for the issue, please feel free to do your own thing.

For background: I don't think Storybook, or the packages with failing `dtslint` runs, `storybook-readme` and `storybook-addon-jsx`, directly depend on `@types/node`, but it's common for frontend packages to install build tooling that does depend on the package.  This causes Node types to be introduced to the default `typeRoot`, so you wind up with Node types merged in with those in the DOM lib.  (Storybook may be a bit of an odd duck, because it looks like in some cases, React declares its own empty DOM interfaces in `@types/react/globals.d.ts`.)

At any rate, I hope the proposed fix works for you.
meyfa added a commit to meyfa/DefinitelyTyped that referenced this pull request Oct 9, 2022
Node.js types are currently out of sync between TS 4.9+ and TS <= 4.8
variants. Specifically, PR DefinitelyTyped#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.
@meyfa meyfa mentioned this pull request Oct 9, 2022
7 tasks
typescript-bot pushed a commit that referenced this pull request Oct 10, 2022
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.
@Jack-Works
Copy link
Contributor

This PR is breaking Vue 3. I'll draft issue in both Vue repo and DefinitelyTyped repo soon.

@Jack-Works
Copy link
Contributor

See #62759 and vuejs/core#6899

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Check Config Changes a module config files Critical package Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet