It seems like types/node is getting included in the types/node/v18 test run. undici-types does /// <reference types="node" /> but I think that should get intercepted by the paths defined in types/node/v18/tsconfig.json. Not sure what’s going on exactly.

I think that should get intercepted by the paths

Type reference directives don’t go through paths 🥴

I have a workaround, but it’s really ugly and requires a tsconfig.json change that’s currently illegal according to dtslint: 30fe632. We need to add a higher priority typeRoots that will have a file or directory named node inside to catch the /// <reference types="node" /> in undici-types. So this commit adds the typeRoot entry and an otherwise useless node.d.ts file 😕

The other thing we can do in the short term is not include fetch typings in v18 until we work out another solution. The real fix is completely restructuring DT into a proper monorepo and not using tsconfig hacks to override module resolution, but that will take a while.

Inspecting the JavaScript source for this package found some properties that are not in the .d.ts files.
The check for missing properties isn't always right, so take this list as advice, not a requirement.

formidable (unpkg)

was missing the following properties:

1. OctetstreamParser
2. QueryStringParser
3. octetstream
4. querystring
5. multipart

Generated by 🚫 dangerJS against e942e80

@andrewbranch Thank you for submitting this PR!

This is a live comment which I will keep updated.

4 packages in this PR

• formidable — on npm, on unpkg
• hidefile — on npm, on unpkg
  • owner-approval: @nelsonni
• levelup — on npm, on unpkg
• node — on npm, on unpkg
  • owner-approval: @mcollina

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 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": 66824,
  "author": "andrewbranch",
  "headCommitOid": "e942e8076f6ef1139d130e2f70516891eb2952b3",
  "mergeBaseOid": "868275941a6a6687b783bdadd80b8caf0d89dc00",
  "lastPushDate": "2023-09-25T19:21:26.000Z",
  "lastActivityDate": "2023-10-09T17:32:11.000Z",
  "mergeOfferDate": "2023-10-09T16:00:14.000Z",
  "mergeRequestDate": "2023-10-09T17:31:40.000Z",
  "mergeRequestUser": "andrewbranch",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "formidable",
      "kind": "edit",
      "files": [
        {
          "path": "types/formidable/formidable-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "Nemo157",
        "martin-badin",
        "devLana"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Popular"
    },
    {
      "name": "hidefile",
      "kind": "edit",
      "files": [
        {
          "path": "types/hidefile/test/hidefile-tests.cjs.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "nelsonni"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "name": "levelup",
      "kind": "edit",
      "files": [
        {
          "path": "types/levelup/levelup-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "MeirionHughes",
        "danwbyrne",
        "carsonfarmer",
        "istherepie"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Popular"
    },
    {
      "name": "node",
      "kind": "edit",
      "files": [
        {
          "path": "types/node/globals.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/package.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/node/test/globals.ts",
          "kind": "test"
        },
        {
          "path": "types/node/ts4.8/globals.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/ts4.8/test/globals.ts",
          "kind": "test"
        }
      ],
      "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",
        "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": [
    {
      "type": "approved",
      "reviewer": "jakebailey",
      "date": "2023-10-09T15:59:25.000Z",
      "isMaintainer": true
    },
    {
      "type": "approved",
      "reviewer": "nelsonni",
      "date": "2023-10-09T04:50:22.000Z",
      "isMaintainer": false
    },
    {
      "type": "stale",
      "reviewer": "SimonSchick",
      "date": "2023-10-07T09:01:20.000Z",
      "abbrOid": "afce591"
    },
    {
      "type": "approved",
      "reviewer": "mcollina",
      "date": "2023-10-07T07:14:59.000Z",
      "isMaintainer": false
    },
    {
      "type": "stale",
      "reviewer": "thw0rted",
      "date": "2023-09-29T13:03:55.000Z",
      "abbrOid": "b6a3e0f"
    },
    {
      "type": "stale",
      "reviewer": "Semigradsky",
      "date": "2023-09-29T07:01:34.000Z",
      "abbrOid": "b6a3e0f"
    },
    {
      "type": "stale",
      "reviewer": "carsonfarmer",
      "date": "2023-09-29T00:32:17.000Z",
      "abbrOid": "b6a3e0f"
    },
    {
      "type": "stale",
      "reviewer": "Ethan-Arrowood",
      "date": "2023-09-25T19:50:11.000Z",
      "abbrOid": "afce591"
    }
  ],
  "mainBotCommentID": 1740155365,
  "ciResult": "pass"
}

🔔 @Nemo157 @martin-badin @devLana @nelsonni @MeirionHughes @danwbyrne @carsonfarmer @istherepie @microsoft @DefinitelyTyped @jkomyno @alvis @r3nya @btoueg @smac89 @Touffy @DeividasBakanas @eyqs @Hannes-Magnusson-CK @hoo29 @kjin @ajafff @islishude @mwiktorczyk @mohsen1 @n-e @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 Approve or Request Changes in the GitHub UI so I know what's going on.

@Ethan-Arrowood Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

@Semigradsky, @Ethan-Arrowood Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

Hi @andrewbranch !

I do land here and I see that the types is quite recent. (2 hours only !)

I'm trying do test things with globalThis.fetch type, but I got this errors :

globalThis.fetch = async (
  input: URL | RequestInfo,
  options: RequestInit | undefined,
) => {};

does output:

Type '(input: URL | RequestInfo, options: RequestInit | undefined) => Promise<Response>' is not assignable to type '{ (input: URL | RequestInfo, init?: RequestInit | undefined): Promise<Response>; (input: RequestInfo, init?: RequestInit | undefined): Promise<...>; }'.
  Types of parameters 'input' and 'input' are incompatible.
    Type 'RequestInfo' is not assignable to type 'URL | RequestInfo'.
      Type 'Request' is not assignable to type 'URL | RequestInfo'.
        Property 'referrer' is missing in type 'import("/home/jdeniau/code/metch-fock/node_modules/undici-types/fetch").Request' but required in type 'Request'.ts(2322)
lib.dom.d.ts(18715, 14): 'referrer' is declared here.

By sniffing the .d.ts file, I found that you sometimes use NodeJS.fetch.XXX, so I tried this:

globalThis.fetch = async (
  input: URL | NodeJS.fetch.RequestInfo,
  options: NodeJS.fetch.RequestInit | undefined,
) => {};

But another issue there:

Type '(input: URL | NodeJS.fetch.RequestInfo, options: NodeJS.fetch.RequestInit | undefined) => Promise<Response>' is not assignable to type '{ (input: URL | RequestInfo, init?: RequestInit | undefined): Promise<Response>; (input: RequestInfo, init?: RequestInit | undefined): Promise<...>; }'.
  Types of parameters 'input' and 'input' are incompatible.
    Type 'URL | RequestInfo' is not assignable to type 'URL | import("/home/jdeniau/code/metch-fock/node_modules/undici-types/fetch").RequestInfo'.
      Type 'Request' is not assignable to type 'URL | RequestInfo'.
        Property 'duplex' is missing in type 'Request' but required in type 'import("/home/jdeniau/code/metch-fock/node_modules/undici-types/fetch").Request'.ts(2322)
fetch.d.ts(154, 12): 'duplex' is declared here.

If I roll back from "@types/node": "20.8.4" to "@types/node": "20.8.3", then the error disappear.

How about

globalThis.fetch = async (input, options) => {
    
}

The types will be inferred from context.

Indeed, it is inferred, but having the types written makes it really easier to navigate into the types while developing.

Furthermore, I do call other methods, for example to extract the url of the input, and I need to type that function. If I want to infer, I will need to do weird TS tricks. Probably something like:

function getInputUrl(input: Parameters<typeof globalThis.fetch>[0]) {

witch is way less readable than

function getInputUrl(input: RequestInfo | URL) {

Pushing this a little forward, I found that input is not an instance of URL:

function getInputUrl(
  input: Parameters<typeof globalThis.fetch>[0],
): string {
  if (typeof input === 'string') {
    return input;
  }

  if (input instanceof URL) {
    return input.toString();
  }

  return input.url;
}

The last return line does throw this error:

Property 'url' does not exist on type 'URL | Request'.
  Property 'url' does not exist on type 'URL'.ts(2339)

Is it a conflict between typescript's libdom.d.ts and @types/node ?

Yes. Users should avoid having @types/node and lib.dom.d.ts at the same time, but this PR jumped through many hoops to ensure that usage of fetch will still work. As someone mocking fetch, I unfortunately expect you’ll have to jump through some hoops as well.

OK, no problem, I will try to do this.

Thanks for your time 👍

Sorry for this notifying a lot of people, I figured the MR would notify less people than the issue, but I can also open a new issue to ask more direct questions and provide more snippets.

@andrewbranch if I want to assign the dispatcher by creating an Agent or MockAgent or call setGlobalDispatcher, should I be importing using undici-types directly?

I know that this issue is primarily focused on fetch itself, but there are other aspects of undici which I use for testing and I was wondering if I just need to keep undici-types as an explicit devDependency, or if I should rely on the dependency that types/node is pulling it in, or some other solution?

Prior to undici-types being created, I have been including undici as a dependency.

Examples:

import { Agent } from "undici-types"

const httpsAgentInsecure = new Agent({
  connect: { rejectUnauthorized: false },
})

fetch(url, {
  method: "GET",
  headers: {},
  dispatcher: httpsAgentInsecure,
}),

import { MockAgent, setGlobalDispatcher } from "undici"

// Mock the fetch requests
const mockAgent = new MockAgent()
setGlobalDispatcher(mockAgent)
mockAgent.disableNetConnect()

You can’t import values from undici-types and actually use them in value-space:

import { Agent } from "undici-types";

new Agent(); // this will crash in Node.js

So you would want to continue to depend on undici, yes.

Hey @andrewbranch! As mentioned in cloudflare/workerd#1298, it’s not your fault but this breaks anything which overrides the fetch global with an incompatible type if they’re doing so through tsconfig.json#types. Cloudflare Workers is a great but not exclusive example of this, since they recommend users add "types": ["@cloudflare/workers-types"] to their tsconfig.json, but if a single one of their dependencies imports @types/node, which is very common (ex. openai-node, stripe-node), they’ll start to get type incompatibilities. Cloudflare can’t fix this because they rely on non-standard extensions to fetch, and it seems reasonable but a huge lift to ask every ecosystem package to remove their @types/node references, which in many cases will involve splitting their entrypoints.

I wanted to raise the idea here (discussed further in this comment) of using TypeScript 5’s customConditions to neutralise @types/node under certain conditions. The idea would be that a user could add "customConditions": ["workerd"] to their compilerOptions to hint to TypeScript that they’re overriding the global types in some other way. Then @types/node would upgrade to use package.json#exports like so:

"exports": {
    "workerd": {
        "node": {
            "types": "./index.workerd.d.ts",
        },
        "types": "./noop.d.ts"
    },
    "types": "./index.d.ts"
}

By default, this neutralises @types/node but would also allow runtimes to operate in Node compatibility mode (which Deno, Bun, and Cloudflare have), where they’d have custom entrypoints that omit anything that would be overridden by their global types. We could even omit types that aren’t available in their respective Node compat modes, but at the risk of introducing a few type incompatibilities that could be corrected with patches and PRs.

(We would also add keys for anything else in WinterCG’s Runtime Keys list, but AFAIK only workerd has fetch incompatibilities right now; other runtimes have their own solutions to this or just remain compatible in ways that Cloudflare can’t).

I think this solution would be reasonable because it would remain backwards-compatible by default and require users to add customConditions in the rare case that they’re affected. I also think it’s a reasonable and idiomatic use of customConditions (although, arguably, the most idiomatic solution is to upgrade the entire package ecosystem). The only problems this would really raise are if a user is running their runtime in Node compatibility mode, and their runtime lacks types for a Node package that one of their dependencies need, but solving this is a much lower lift than adding exports conditions to every package.

As a bonus, this could also solve a lot of the concerns and hangups in microsoft/TypeScript#18588 w/r/t browser packages accidentally polluting their namespaces with @types/node.

I’m not asking to solve this immediately, but would the @types/node maintainers be open to something like this?

(BTW I don’t work for Cloudflare; just a concerned user :P)

There’s kind of a fundamental problem with any approach that removes types from a program. While it’s true that sometimes users wind up with @types/node in their compilation for absolutely no good reason and it can be fully removed without consequence, more often you have something like this:

// user code running on Cloudflare
import { querySmartAI } from "openai-node";
const res = querySmartAI("what is the melting point of mushrooms (my grandma used to tell me)");

// node_modules/openai-node/index.d.ts
/// <reference types="node" />
export function querySmartAI(query: string): NodeJS.ReadableStream;

If you override @types/node to resolve to a no-op, that will cause errors in the user’s program. Even if they can be silenced with skipLibCheck, their res will be infected with type any from the error. So which types should be omitted and which included in a given combination of import conditions? I think any viable solution needs to put the responsibility of answering that question on the target runtime, not the types package that’s being accidentally included.

Thanks for explaining a bit more of the motivation there! I also liked your comment in the original issue, which really gave me pause to think.

Perhaps I should just back up and ask what you think the idiomatic solution looks like? (You have much much more insight into what TypeScript think about this than I do 😅)

• Can you explain what a runtime-led solution would look like? I am having a hard time understanding what role they can play here beyond maintaining their own patched fork of @types/node—is that what you mean? (And in that case, is it a stretch for them to just maintain a conditional export here?)
  • Given your point above, would they deliberately type unavailable APIs as never or unknown instead of omitting them?
• …or does it look more like playing whack-a-mole with packages as they upgrade to v20.8.4? (Does this look like each package adding export conditions for types? Is the size of that effort across the entire ecosystem larger than the above option?)
• Something else?

I honestly don’t know what the best way forward would be, but I think patching @types/node and having users replace it via package manager functionality sounds the most promising.

Given your point above, would they deliberately type unavailable APIs as never or unknown instead of omitting them?

No, I think it would be correct to omit them. My point was only that (1) omitting all of @types/node is probably not correct, but (2) I don’t want @types/node to be responsible for knowing how much of Cloudflare’s worker API overlaps with Node.js.

Can we please get an intended usage example for this? Simply adding "node" to "types" in tsconfig.json does not expose global Fetch API types to the code. I've tried "types": ["undici-types"] in all shapes and forms to no avail either.

There shouldn't be anything special; it "just works" for me in a project where I just did:

$ npm init --yes
$ npm install -D typescript @types/node
$ npx tsc --init
$ echo "fetch()" > index.ts

(And set lib to ensure DOM doesn't show up.)

Apologies, forgot to also set lib: ["es2016"]:

This PR exposed RequestInit globally but missed other "Init" typedef/dictionary types that lib-dom exposes globally, which will cause compatibility errors like mswjs/msw#1836 for isomorphic libraries. I believe we're still missing:

• ResponseInit
• BodyInit
• HeadersInit
• RequestInfo
• XMLHttpRequestBodyInit (actually only used in BodyInit definition?)

Some of these are very simple structurally but if they don't exist in the global namespace, any library that refers to them will break, sometimes in confusing ways.

ETA: It looks like ResponseInit is the only one on my list that's an interface not a type. I don't think we ever figured out how to handle "conditional" global types, did we?

1000 * ❤️

I'm still having to directly depend on udici-types to be able to use these runtime values, which feels dirty. Is it necessary?

import { Dispatcher, getGlobalDispatcher, setGlobalDispatcher, ProxyAgent } from "undici-types";

The import you wrote should fail at runtime because undici-types only exports the type side of things. I haven't tried to use Dispatcher / ProxyAgent but they don't appear to actually be exposed at runtime in Node (yet?): nodejs/node#43187

This fails when building under an ESNext module. Requiring to use NodeNext.

This fails when building under an ESNext module. Requiring to use NodeNext.

I am using ESNext module and it works well. Make sure you are using the latest version of @types/node

"lib": ["es2022"],
"module": "ESNext",
"moduleResolution": "Bundler",