-
Notifications
You must be signed in to change notification settings - Fork 300
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
Comments
DiagnosisIt 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 This is a big and hard to solve problem, because TypeScript automatically emits Unfortunately, TypeScript can’t fix this problem without a much bigger rearchitecture (microsoft/TypeScript#18588; the Aside: A better, cheap workaroundA slightly better workaround than the above would be to add the following to a declare module "@types/node" {
declare const value: unknown;
export default value;
} This will limit the damage of neutralising The long-term, idiomatic solutionsTypeScript 5.0 includes a new However, as we’ve found with the "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 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 patchFirst, add this to your {
"compilerOptions": {
"customConditions": ["workerd", "worker", "browser"],
}
} Second, patch "exports": {
"workerd": {
"types": "./noop.d.ts"
},
"types": "./index.d.ts"
} And add
|
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 |
If you use |
Yes that might be a possible workaround, with loading |
Reading Andrew’s comments, it looks like the way forward is for Cloudflare (or the community) to maintain their own fork of |
Now that
I have gone with (3), which looks like ( "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 |
The override is necessary due to cloudflare/workerd#1298
See also cloudflare/workerd#1298 for more information
I am trying to get a similar patch, is this the most up to date version? |
Should work, are you having any problems with it? I also gave instructions on how to re-create the patch for any |
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
|
Could you please explain what you’re trying to achieve in a broader sense? The error you have posted occurs because 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 It sounds like you might trying to do the opposite, which is to import the Node types (remember, you will also need to enable 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. |
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 :) |
Thanks! No worries, I appreciate your time @huw thanks |
For anyone else running into this issue here is what I am trying to solve.... Problem - type conflictsWe keep getting type conflicts within our edittors and when trying to build our project.
The issue is because there are two version of Response/Request and fetch that were added to node types 20^. SolutionsI 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
or for yarn...
Thank you all for your help! |
@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 So our patch looks like this (as you can't mix 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"
+ }
} |
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.
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>
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. |
Potentially could be fixed if we generate node types on the fly based on node compat flags (aka Types as a Service). |
This is a workaround for cloudflare/workerd#1298
* 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
Hello! Just to say that using overwrite or resolutions in So we are basically blocked currently 😅 Related nx issue: nrwl/nx#22998 |
@Lp-Francois - I think you are saying that Nx requires Node.js 20.12.2 or above to work (at runtime). |
Hey @petebacondarwin, it gives me "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 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 🤔 |
FYI, TypeScript 5.6 will pretty much require a newer version of |
I struggled with this issue for a whole day and resolved it by using the Don't forget to uninstall {
"compilerOptions": {
"types": ["./.wrangler/types/runtime.d.ts"]
}
} |
Sadly this is only really a solution if you're not using |
Closing as this is now fixed. |
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 |
@prblackburn the workaround still exists, because it assumes that you are using |
Transitive loading of @types/node overrides @cloudflare/workers-types. https://developers.cloudflare.com/workers/languages/typescript/#transitive-loading-of-typesnode-overrides-cloudflareworkers-types cloudflare/workerd#1298
As per DefinitelyTyped/DefinitelyTyped#66824 which was merged recently and released in
@types/node@20.8.4
, Node.js now defines its ownRequest
,Response
, etc. types viaundici-types
.While this is great in Node environments, it completely messes with the
Request
andResponse
(maybe others?) types defined by@cloudflare/workers-types
.Example code that used to work before:
Now has a type error with:
Expected 0 type arguments, but got 1.
.Or:
Now has a type error with `Property 'cf' does not exist on type 'Request'.
Or:
Now has a type error with:
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 mypackage.json
:It's a very naive and short-term workaround, but works for now.
The text was updated successfully, but these errors were encountered: