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

Transitive loading of @types/node breaks Request/Response (etc.) types #1298

Closed
Cherry opened this issue Oct 10, 2023 · 28 comments
Closed

Transitive loading of @types/node breaks Request/Response (etc.) types #1298

Cherry opened this issue Oct 10, 2023 · 28 comments
Labels
nodejs compat types Related to @cloudflare/workers-types

Comments

@Cherry
Copy link
Contributor

Cherry commented Oct 10, 2023

As per DefinitelyTyped/DefinitelyTyped#66824 which was merged recently and released in @types/node@20.8.4, Node.js now defines its own Request, Response, etc. types via undici-types.

While this is great in Node environments, it completely messes with the Request and Response (maybe others?) types defined by @cloudflare/workers-types.

Example code that used to work before:

type SomeType = {
	foo: string
};
const response = await fetch(...);
const json = await response.json<SomeType>()

Now has a type error with: Expected 0 type arguments, but got 1..

Or:

const country = request?.cf?.country;

Now has a type error with `Property 'cf' does not exist on type 'Request'.

Or:

new Request(..., request);

Now has a type error with:

Argument of type 'Request' is not assignable to parameter of type 'RequestInit'.
  Types of property 'referrerPolicy' are incompatible.
    Type 'string' is not assignable to type 'ReferrerPolicy | undefined'.

Why load @types/node?

This is a fair question, and you generally don't need to load @types/node at all, and in fact, I don't directly. But, many dependencies that people use, do in fact require @types/node, including the likes of @types/pg, which is loaded by @neondatabase/serverless.

It seems that once this newer version of @types/node is loaded into memory at all, it just overrides the globally defined Request/Response/fetch types from @cloudflare/workers-types.

Full reproduction

See the following github repo: https://github.com/Cherry/workers-node-types-issue-repro.

This showcases an example package that loads @types/node, and if you comment in/out the import you'll see the type errors now show up.

Workaround

My current workaround for this is to pin @types/node via npm overrides, with this in my package.json:

{
	"overrides": {
		"@types/node": "20.8.3"
	}
}

It's a very naive and short-term workaround, but works for now.

@Cherry Cherry added the types Related to @cloudflare/workers-types label Oct 10, 2023
@huw
Copy link

huw commented Oct 11, 2023

Diagnosis

It appears that this ends up running into some fundamental TypeScript issues. I am not an expert here, but what I understand is happening is that TypeScript imports your tsconfig.json types field first, then overrides it with anything imported from a dependency. If you have even a single dependency that overrides a global type that you’re using from @cloudflare/workers-types, then too bad, that type is overridden.

This is a big and hard to solve problem, because TypeScript automatically emits /// <reference types="node" /> any time a dependency uses a Node global. My pnpm why @types/node is showing tens of projects with these globals; it’s infeasible for me to personally patch or PR each of these dependencies to fix their projects, even if you buy that those projects have made some mistake in the first place.

Unfortunately, TypeScript can’t fix this problem without a much bigger rearchitecture (microsoft/TypeScript#18588; the strict-env proposal has also stalled). If there’s any silver lining to this, I guess it’s that it would also break other WinterCG projects that use TypeScript like this (IIRC the only one would be @edge-runtime/types, and only if they have a similar type incompatibility; Deno and others do their global type checking differently or just use @types/node).

Aside: A better, cheap workaround

A slightly better workaround than the above would be to add the following to a global.d.ts in your project:

declare module "@types/node" {
  declare const value: unknown;
  export default value;
}

This will limit the damage of neutralising @types/node to just the projects you’re using Workers in, and allow you to keep @types/node unpinned for actual node projects (such as scripts or other packages in a monorepo).

The long-term, idiomatic solutions

TypeScript 5.0 includes a new compilerOptions directive, customConditions. This means the compiler can now resolve package.json exports conditions to locate types depending on the runtime/conditions that are present. In short, this means that packages which emit a /// <reference types="node" /> can scope this to just be imported by the node condition, or to remove it for the workerd condition. This is the TypeScript equivalent of a bundler’s conditions directive (ex. ESBuild), and you use it in much the same way.

However, as we’ve found with the workerd condition in the ecosystem, this requires every package to update to split their exports. I’m not convinced this is feasible. An easier solution would be for @types/node to add a workerd condition with a compatible export, like so:

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

By default, this would just be a no-op, which trusts that you’re importing @cloudflare/workers-types and neutralises @types/node. If you’re running Workers by default, this would most accurately reflect the types you’d have access to. But the beauty of this solution is that if you’re running in Node compatibility mode, you could specify "customConditions": ["workerd", "node"] and we’d serve up a version with all of the incompatible types removed. This would most accurately reflect Node compat mode, where we want a lot of the proper node types, but Cloudflare-augmented globals would still be present. We could even omit anything that’s not supported in Node compat mode—but I assume Cloudflare would be responsible for maintaining this file.

I’ll hit up the DefinitelyTyped team and see what they think—an opinion from Cloudflare would be greatly appreciated here also :)

TL;DR just give me a patch

First, add this to your tsconfig.json:

{
    "compilerOptions": {
        "customConditions": ["workerd", "worker", "browser"],
    }
}

Second, patch @types/node/package.json using your package manager to add:

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

And add @types/node/noop.d.ts to your patch—it just needs to be empty. Here’s a patchfile for pnpm:

diff --git a/index.workerd.d.ts b/index.workerd.d.ts
new file mode 100644
index 0000000000000000000000000000000000000000..e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
diff --git a/package.json b/package.json
index e9e15c90021711122a01a324f07f3675e13e1e14..b04e27142c8ef196d8bb3bb53661794fde3569e7 100644
--- a/package.json
+++ b/package.json
@@ -213,6 +213,12 @@
     ],
     "main": "",
     "types": "index.d.ts",
+    "exports": {
+        "workerd": {
+            "types": "./index.workerd.d.ts"
+        },
+        "types": "./index.d.ts"
+    },
     "typesVersions": {
         "<=4.8": {
             "*": [

@Cherry
Copy link
Contributor Author

Cherry commented Oct 11, 2023

This is fantastic insight, thank you so much for the detailed response!

I'm unfortunately in a project where I do value having the types for node:crypto, so noop-ing everything is less than ideal, but this seems like a better workaround for folks who don't need any Node types for the nodejs_compat flag.

@Skye-31
Copy link

Skye-31 commented Oct 11, 2023

I'm unfortunately in a project where I do value having the types for node:crypto, so noop-ing everything is less than ideal, but this seems like a better workaround for folks who don't need any Node types for the nodejs_compat flag.

If you use "types": ["@cloudflare/workers-types", "node/crypto"] alongside the above workaround, you might be able to get this to work?

@Cherry
Copy link
Contributor Author

Cherry commented Oct 11, 2023

I'm unfortunately in a project where I do value having the types for node:crypto, so noop-ing everything is less than ideal, but this seems like a better workaround for folks who don't need any Node types for the nodejs_compat flag.

If you use "types": ["@cloudflare/workers-types", "node/crypto"] alongside the above workaround, you might be able to get this to work?

Yes that might be a possible workaround, with loading @types/node/crypto, @types/node/buffer etc. explicitly.

@huw
Copy link

huw commented Oct 16, 2023

Reading Andrew’s comments, it looks like the way forward is for Cloudflare (or the community) to maintain their own fork of @types/node with the above changes, and encouraging users to override their dependencies. I think it would be enough for that package to just /// <reference types="node" /> for index.node.d.ts, and have a blank file at index.workerd.d.ts, in which case it could just have a peer dependency on @types/node and never have to update its resolutions? But I’m not super well-versed with package magic.

@huw
Copy link

huw commented Oct 27, 2023

Now that @types/node v18 has been updated to include fetch types, I suspect this issue will show up a lot more frequently. So it’s worth also adding that there are a few ways to handle having multiple versions of @types/node in your project that need patching:

  1. Patch every version (boring, slow)
  2. Frequently run pnpm -r upgrade @types/node (or equivalent) to upgrade to the latest satisfiable versions. In my monorepo this got me down to a patch for v18 and v20
  3. Pin @types/node to the version you’re using and just patch it once

I have gone with (3), which looks like (package.json):

"pnpm": {
  "patchedDependencies": {
    "@types/node@20.8.9": "patches/@types__node@20.8.9.patch"
  },
  "overrides": {
    "@types/node": "^20.0.0"
  }
},

For me, this means updating the patch whenever @types/node updates, but usually that just involves renaming the file cause their package.json isn’t going to change a tonne.

AdiRishi added a commit to AdiRishi/turborepo-remote-cache-cloudflare that referenced this issue Nov 7, 2023
kulla added a commit to serlo/cloudflare-worker that referenced this issue Nov 20, 2023
@chitalian
Copy link

I am trying to get a similar patch, is this the most up to date version?

@huw
Copy link

huw commented Dec 13, 2023

Should work, are you having any problems with it? I also gave instructions on how to re-create the patch for any @types/node version if it’s not working…

@chitalian
Copy link

Thanks @huw for getting back so quickly

Here is what I have so far outlined in this PR: https://github.com/Helicone/helicone/pull/1140/files

However, it is still not finding the Node types

src/lib/secureCache.ts:51:28 - error TS2591: Cannot find name 'Buffer'. Do you need to install type definitions for node? Try `npm i --save-dev @types/node` and then add 'node' to the types field in your tsconfig.

51   const encryptedContent = Buffer.from(encrypted.content, "hex");
                              ~~~~~~

@huw
Copy link

huw commented Dec 13, 2023

Could you please explain what you’re trying to achieve in a broader sense?

The error you have posted occurs because secureCache.ts is trying to use Buffer, which is a Node-only API. This API will not run in Cloudflare Workers unless you enable node_compat or nodejs_compat. You will either need to enable one of those flags or remove the dependency on Buffer to get your code to run.

This issue is about something different, which is that sometimes when importing third-party libraries, they will also import types from Node into the global typespace, which can cause code to type-check correctly when it shouldn’t (for example, if you wrote code with Buffer but you wanted to run it in Workers, this code might type-check fine even if it can’t actually run!). You can use the patch above to improve the correctness of your type-checking, but this will have no bearing on whether the libraries you depend on will actually need Buffer in the first place.

It sounds like you might trying to do the opposite, which is to import the Node types (remember, you will also need to enable node_compat or nodejs_compat in your wrangler.toml). You can do that by adding types: ["node"] to your tsconfig.json#compilerOptions.

If this doesn’t make sense or you’re having more problems, can I ask you to ask this in the Cloudflare Discord? I can’t be 100% sure without more context, but it doesn’t sound like your problem is relevant to this issue.

@chitalian
Copy link

@huw Happy to chat more on discord but that link seems to be broken.

Can you friend me? #justin75 and we can pick up from there?

@huw
Copy link

huw commented Dec 13, 2023

This link should work, but just so we're clear I don't have the time to help you there (I don't work for Cloudflare just FYI), I'm just redirecting you to the right place to ask for it :)

@chitalian
Copy link

Thanks! No worries, I appreciate your time @huw thanks

@chitalian
Copy link

chitalian commented Dec 13, 2023

For anyone else running into this issue here is what I am trying to solve....

Problem - type conflicts

We keep getting type conflicts within our edittors and when trying to build our project.

image

src/lib/providerCalls/call.ts:63:3 - error TS2741: Property 'webSocket' is missing in type 'import("/Users/justin/Documents/repos/promptzero/helicone/worker/node_modules/.pnpm/undici-types@5.26.5/node_modules/undici-types/fetch").Response' but required in type 'import("/Users/justin/Documents/repos/promptzero/helicone/worker/node_modules/.pnpm/@cloudflare+workers-types@4.20231121.0/node_modules/@cloudflare/workers-types/index").Response'.

63   return response;
     ~~~~~~

  node_modules/.pnpm/@cloudflare+workers-types@4.20231121.0/node_modules/@cloudflare/workers-types/index.ts:1073:12
    1073   readonly webSocket: WebSocket | null;
                    ~~~~~~~~~
    'webSocket' is declared here.

The issue is because there are two version of Response/Request and fetch that were added to node types 20^.

image

Solutions

I tried to use @huw's solution here however, I was still getting a ton of different compile and type checking issues.

using pnpm to override the node types version to 18 works for now but this is not a long term solution

  "pnpm": {
    "overrides": {
      "@types/node": "18.15.3"
    }
  },

or for yarn...

  "resolutions": {
    "@types/node": "18.15.3"
  },

Thank you all for your help!

@smcgivern
Copy link

@huw thanks for #1298 (comment), that was very helpful.

Just a note for anyone else who runs into this: some libraries (like Next.js) try to find a package's package.json, and this will fail with this patch. By default, package.json is considered to be exported, but if there is an explicit exports entry, it needs to be manually included there: nodejs/node#33460 / uuidjs/uuid#444

So our patch looks like this (as you can't mix ./ and prefixless exports):

diff --git a/noop.d.ts b/noop.d.ts
new file mode 100644
index 0000000000000000000000000000000000000000..e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
diff --git a/package.json b/package.json
index c902b012556ed4bca4b8038a0f4523ce3cb76287..07f730b9d31475f59e108de66e832122ee57c3c6 100644
--- a/package.json
+++ b/package.json
@@ -226,5 +226,12 @@
     },
     "typesPublisherContentHash": "e270b1ec6520966a2cb0578fa33386344f78258bc0bd109a3e4b315d052e2b62",
     "typeScriptVersion": "4.5",
-    "nonNpm": true
+    "nonNpm": true,
+    "exports": {
+        "./workerd": {
+            "./types": "./noop.d.ts"
+        },
+        "./types": "./index.d.ts",
+        "./package.json": "./package.json"
+    }
 }

jrhender added a commit to StampyAI/stampy-ui that referenced this issue Feb 10, 2024
stampy.ts tests were failing (e.g. https://github.com/StampyAI/stampy-ui/actions/runs/7854687158/job/21435971107) with the following error:

```
    app/server-utils/stampy.ts:196:45 - error TS2576: Property 'json' does not exist on type 'Response'. Did you mean to access the static member 'Response.json' instead?

    196     json = await (await fetch(url, params)).json()
```

Removed @nodes/types as I think it was due to the types conflict described cloudflare/workerd#1298.
gabivlj added a commit to cloudflare/serverless-registry that referenced this issue Mar 1, 2024
gabivlj added a commit to cloudflare/serverless-registry that referenced this issue Mar 1, 2024
gabivlj added a commit to cloudflare/serverless-registry that referenced this issue Mar 1, 2024
gabivlj added a commit to cloudflare/serverless-registry that referenced this issue Mar 1, 2024
harryzcy pushed a commit to harryzcy/serverless-registry that referenced this issue Mar 1, 2024
gabivlj added a commit to cloudflare/serverless-registry that referenced this issue Mar 4, 2024
History of this squashed commit:
* Fix a test and type error

* Add github action tests

* Pass test in both node 18 and 20

* Remove some unused imports

* Add typecheck and build to CI

* Mute some type errors

* chore: fix type issues found after upgrading dependencies, see: cloudflare/workerd#1298

* Remove ts-nocheck

* Still disable threads for vitest to fix timeouts

* fix: simplify manifest upload by copying into a text

Locally in unit tests manifest upload could hang sometimes.

* Remove `threads: false` in vitest config

* Set pool to forks in vitest config

* Avoid pool option and avoid pining node types

---------

Co-authored-by: Gabi Villalonga Simon <gvillalongasimon@cloudflare.com>
Co-authored-by: Gabriel Villalonga Simón <gabitriqui@gmail.com>
@punkeel
Copy link
Member

punkeel commented Apr 7, 2024

Hitting this while migrating my Workers from Service Workers to ESM (by force, not choice), and in turn having to migrate from Jest to Vitest. This is painful.
The docs already recommend pinning a specific Vitest version, do we also need to pin @types/node?

@irvinebroque
Copy link
Collaborator

@petebacondarwin
Copy link
Contributor

Potentially could be fixed if we generate node types on the fly based on node compat flags (aka Types as a Service).

jahands added a commit to jahands/serverless-registry that referenced this issue Jun 19, 2024
gabivlj pushed a commit to cloudflare/serverless-registry that referenced this issue Jun 19, 2024
* chore: Remove package-lock.json and yarn.lock in favor of pnpm-lock.yaml

Maintaining multiple lock files adds unnecessary complexity to the project

* chore: Update pnpm lockfile to 9.0

* chore: Update wrangler and workers-types

* docs: Use npx when running wrangler

Use of global wrangler installation is not recommended.

* fix: Install pnpm in ci

* fix: Override @types/node to prevent conflict with workers-types

This is a workaround for cloudflare/workerd#1298
@Lp-Francois
Copy link

Hello! Just to say that using overwrite or resolutions in package.json is not working for us, as we use a monorepo (nx) and they have a bug that requires a nodejs version at least 20.12.2 :( (which conflicts with the v20.8.3 advised here)

So we are basically blocked currently 😅

Related nx issue: nrwl/nx#22998

@petebacondarwin
Copy link
Contributor

@Lp-Francois - I think you are saying that Nx requires Node.js 20.12.2 or above to work (at runtime).
The workaround suggested is only about locking down the "types" of Node.js to 20.8.3.
Are you saying that locking the types is somehow breaking your builds when using Nx?

@Lp-Francois
Copy link

Hey @petebacondarwin, it gives me No matching export in "../../../node_modules/@cloudflare/workers-types/index.ts" for import "Response" when starting locally my worker using wrangler.

  "resolutions": {
    "@types/node": "20.8.3"
  }

I tried using 20.8.1, 20.8.3, 20.9.0 for the types and 20.8.1, 20.9.0, 20.12.2 as node version (with a yarn install in between). I thought the runtime version could be an issue :/

Also, as we have different projects with nx others than cloudflare workers, I am afraid than a global override in a monorepo is not the best option, as it could cause type issues 🤔

@jakebailey
Copy link

FYI, TypeScript 5.6 will pretty much require a newer version of @types/node to pick up a number of iterator type updates (iterator methods / improved correctness); the workaround of pinning back to an old version is not going to be viable after that.

@chalkygames123
Copy link

chalkygames123 commented Aug 27, 2024

I struggled with this issue for a whole day and resolved it by using the --experimental-include-runtime option added to the wrangler types command in Wrangler v3.66.0 (the doc incorrectly says 3.66.1).

Don't forget to uninstall @cloudflare/workers-types and add the path of the generated runtime types file to the compilerOptions.types field in your project's tsconfig.json file:

{
	"compilerOptions": {
		"types": ["./.wrangler/types/runtime.d.ts"]
	}
}

@Cherry
Copy link
Contributor Author

Cherry commented Aug 27, 2024

I struggled with this issue for a whole day and resolved it by using the --experimental-include-runtime option added to the wrangler types command in Wrangler v3.66.0 (the doc incorrectly says 3.66.1).

Sadly this is only really a solution if you're not using nodejs_compat and any Node.js APIs. The --experimental-include-runtime option doesn't include these types, so you still have to load @types/node. Or, if something else you're loading includes these, the problem as described in the original issue still persists.

@andyjessop
Copy link
Contributor

@Cherry this issue is being fixed here: #2708

@andyjessop
Copy link
Contributor

Closing as this is now fixed.

@prblackburn
Copy link

If this is considered fixed, may I ask for total clarity if this workaround that still exists in the cloudflare docs is no longer deemed necessary? https://developers.cloudflare.com/workers/languages/typescript/#transitive-loading-of-typesnode-overrides-cloudflareworkers-types

@andyjessop
Copy link
Contributor

@prblackburn the workaround still exists, because it assumes that you are using @cloudflare/workers-types. So unless you are moving to wrangler types --experimental-include-runtime to generate your runtime types, then you will need this suggested workaround.

rakyi added a commit to owid/owid-grapher that referenced this issue Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nodejs compat types Related to @cloudflare/workers-types
Projects
Archived in project
Development

No branches or pull requests