sindresorhus / got

🌐 Human-friendly and powerful HTTP request library for Node.js
MIT License
14.27k stars 935 forks source link

tsc can't resolve types for got 13 #2267

Closed trim21 closed 11 months ago

trim21 commented 1 year ago

Describe the bug

got 13 types exports breaks tsconfig.json "moduleResolution": "Node" and package.json "type": "module"

Actual behavior

index.ts:1:22 - error TS2307: Cannot find module 'got' or its corresponding type declarations.

1 import * as got from 'got'
                       ~~~~~

Found 1 error in index.ts:1

Expected behavior

should resolve types

...

Code to reproduce

import * as got from 'got'

re-produce repo:

https://github.com/trim21/got-type

Checklist

fix:

--- a/package.json
+++ b/package.json
@@ -6,8 +6,8 @@
    "repository": "sindresorhus/got",
    "funding": "https://github.com/sindresorhus/got?sponsor=1",
    "type": "module",
+   "types": "./dist/source/index.d.ts",
    "exports": {
-       "types": "./dist/source/index.d.ts",
        "default": "./dist/source/index.js"
    },
    "engines": {
sindresorhus commented 1 year ago

Got has been an ESM package since v12. You must have "module": "node16", "moduleResolution": "node16" in your tsconfig.

sindresorhus commented 1 year ago

import * as got from 'got'

Got is a default export:

import got from 'got';
trim21 commented 1 year ago

saidly, I can't do that... I'd like to use "moduleResolution": "node16" but there are some packages I'm using doesn't support it...

lucgagan commented 1 year ago

Heads up that node16 module resolution does not work in Next.js projects.

trim21 commented 1 year ago

You only need node16 if you run code code directly generated by tsc, or you are using ts-node.

for example, tsc --outDir dist && node ./dist/index.js or ts-node-esm ./src/index.ts

Another sulution is to upgrade typescript to 5.0 and set moduleResolution to bundler.

This works if you are using any tools to transpile or bundle your server project source files (including nodejs esm loader @esbuild-kit/esm-loader, webpack, rollup, esbuild ...etc).

for examaple, if you are using @esbuild-kit/esm-loader to run your TypeScript esm project directly, or you build your TS project with rollup and run the generated javascript files, you should set moduleResolution to bundler.

This is beacuse nodejs esm loader can be more powerful then nodejs cjs hook. If you are using bundler or esm-loader (@esbuild-kit/esm-loader or tsx), your are not actually using node16 esm moduleResolution, you are using the module resolution algorithm provided by the esm-loader or bundler.

But sadly, ts-node-esm does nothing but transpile, so you will need node16 if you are using ts-node.

asfo commented 1 year ago

I hope that the fix goes in, we are not able to use got on v13 without that fix in our current project, but with the fix everything works well.

Hacksore commented 1 year ago

So in the context of nextjs I'm not entirely sure there is a good path to support moduleResolution: node16.

I created two branches with examples in the following repo one without ESM reproducing the similar issue we see here and one with ESM/node16. https://github.com/Hacksore/got-13-type-woes/tree/master https://github.com/Hacksore/got-13-type-woes/tree/use-node16

The second branch is where I attempted to follow what @sindresorhus was saying and update the tsconfig.json.

With the ESM port I get errors when importing from the next package.

use-node16 ✔ $ yarn build
yarn run v1.22.19
$ next build
- warn Compiled with warnings

./node_modules/keyv/src/index.js
Critical dependency: the request of a dependency is an expression

Import trace for requested module:
./node_modules/keyv/src/index.js
./node_modules/cacheable-request/dist/index.js
./node_modules/got/dist/source/core/index.js
./node_modules/got/dist/source/index.js
./src/app/api/bug/route.ts

- info Linting and checking validity of types .Failed to compile.

./src/app/page.tsx:1:19
Type error: Cannot find module 'next/image' or its corresponding type declarations.

> 1 | import Image from 'next/image'
    |                   ^
  2 | import styles from './page.module.css'
  3 | 
  4 | export default function Home() {
error Command failed with exit code 1.

related next issue https://github.com/vercel/next.js/issues/46078

lucgagan commented 1 year ago

For context, my attempt at adding node16 support to next.js

https://github.com/vercel/next.js/pull/50357

I don't think it is possible to make it work using the current setup.

igorovic commented 1 year ago

I am wondering what could go wrong with this solution ?

{
    "name": "got",
    "version": "13.0.0",
    "description": "Human-friendly and powerful HTTP request library for Node.js",
    "license": "MIT",
    "repository": "sindresorhus/got",
    "funding": "https://github.com/sindresorhus/got?sponsor=1",
    "type": "module",
    "types": "./dist/source/index.d.ts",
    "exports": {
        "types": "./dist/source/index.d.ts",
        "default": "./dist/source/index.js"
    },

Until got version 12.6.0 types attribute was present in package.json. image

In version 13.0.0 it was removed.

If I manually restore this line it works with nextjs 13.

Is there any potential issue restoring the types attribute ?

aPoCoMiLogin commented 1 year ago

this change also breaks webstorm autocomplete

Stanzilla commented 1 year ago

My builds also started failing on v13 with error TS2307: Cannot find module 'got' or its corresponding type declarations.

tzbo commented 1 year ago

Same problem

ClementValot commented 1 year ago

It also breaks tsoa route generation which makes use of the typescript library TypeResolvers and isIndexedAccessTypeNode-like methods

 GenerateMetadataError: Could not determine the keys on PlainResponse
At: myProject\src\MyRoute.ts:28:28.
This was caused by 'statusCode: PlainResponse['statusCode'];'
    at TypeResolver.resolve (myProject\node_modules\@tsoa\cli\dist\metadataGeneration\typeResolver.js:314:23)

EDIT: It works if I replace "moduleResolution": "NodeNext" by "moduleResolution": "Node16"

svsool commented 1 year ago

same problem as well

FabianFrank commented 1 year ago

Got has been an ESM package since v12. You must have "module": "node16", "moduleResolution": "node16" in your tsconfig.

v12 works with moduleResolution: "node" in ESM projects, v13 does not and requires node16.

datner commented 1 year ago

ESM is still dead in the water for 99% of real-world projects. Which is an improvement from 100% of like a year back. But it doesn't mean you have to skip updating got. This is my tsconfig:

{
  "$schema": "https://json.schemastore.org/tsconfig",
  "compilerOptions": {
    "baseUrl": ".",
    "module": "CommonJS",
    "moduleResolution": "node",
    "isolatedModules": true,
    "esModuleInterop": true,
    "strict": true,
    "downlevelIteration": true,
    "paths": {
      "~/*": [
        "./src/*"
      ],
      "got": ["./node_modules/got/dist/source"]
    }
  }
}
terchris commented 1 year ago

Did the required changes to get got package working. But now the packages slugify and sizeOf don't work.

"module": "node16", // Changed this line from "ESNext" to "node16" To use got package
"moduleResolution": "node16", // Changed this line from "node" to "node16" To use got package

Hacksore commented 1 year ago

@datner's solution will work for nextjs as long as you are not converting next to use ESM.

You will still get the warning:

- warn Compiled with warnings

./node_modules/keyv/src/index.js
Critical dependency: the request of a dependency is an expression

Import trace for requested module:
./node_modules/keyv/src/index.js
./node_modules/cacheable-request/dist/index.js
./node_modules/got/dist/source/core/index.js
./node_modules/got/dist/source/index.js
./src/app/api/bug/route.ts

example source

datner commented 1 year ago

converteting next to use ESM.

as far as I know, nextjs do not support and are not planning or intending to support esm, so I am not surprised that got and most probably others won't work without more extensive hacking 😄

in general, it's really quite confusing that everyone supports the import syntax or browser es modules. But practically no one supports node16 esm

trim21 commented 1 year ago

or you can patch got

datner commented 1 year ago

@trim21 share the patch then

trim21 commented 1 year ago

@trim21 share the patch then

read the issue description again

Hacksore commented 1 year ago

Here is the patch-package example

https://github.com/Hacksore/got-13-type-woes/tree/use-patch-package

datner commented 1 year ago

oops!

ClementValot commented 1 year ago

I'm still confused as to why "moduleResolution": "NodeNext" would cause compatibility issues compared to Node16

The documentation is rather scarce on the subject but tends to suggest NodeNext includes Node16 behavior, is there anyone more seasoned on the subject who can please help me understand?

igorovic commented 1 year ago

My undestanding is that for NodeJs the following is perfectly fine.

"type": "module",
"exports": {
    "types": "./dist/source/index.d.ts",
    "default": "./dist/source/index.js"
},

It tells nodejs that this is a esm package. Actually Nodejs in this case does not care about the types information.

However, from typescript perspective types deifinitions are relevant. And in this case TS thinks it's only an esm pacakge and there is probably some module resolution conflicts in Nextjs environment.

Since only TS cares about types I think patching package.json like in the snippet below should be fine.

"type": "module",
"types": "./dist/source/index.d.ts",
"exports": {
    "types": "./dist/source/index.d.ts",
    "default": "./dist/source/index.js"
},

With this patch, I assume that TS will fallback to search types definitions in the root property types when it finds that it's not in a type=module environment. Which is the case when using NextJS. This is actually the patch I use.

Another possible solution would be to define both exports. for esm and for commonjs. Something like:

"type": "module",
    "exports": {
        ".": {
            // Entry-point for `import "my-package"` in ESM
            "import": {
                // Where TypeScript will look.
                "types": "./dist/source/index.d.ts",
                // Where Node.js will look.
                "default": "./dist/source/index.js"
            },
            // Entry-point for `require("my-package")` in CJS
            "require": {
                // Where TypeScript will look.
                "types": "./dist/source/index.d.ts",
                // Where Node.js will look.
                "default": "./dist/source/index.js"
            },
        }
    },

I haven't tested it.


References

https://nodejs.org/api/packages.html Typescript doc

JaneJeon commented 1 year ago

Seeing this as well, even with a vanilla js workspace w/ "type": "module" set (not using typescript, it's just that type completion doesn't work at all for the same reasons outlined above). Suspecting it has to do with the lack of "types" specification in package.json :(

Pipe-Runner commented 1 year ago
"got": ["./node_modules/got/dist/source"]

This seems to unblock the build on Next13 for me. Thank you.

alvis commented 1 year ago

We should continue to push the wave of ESM, but removing the types field doesn't seem to be the way for me. There are just so many constraints in the real world that disallow us to use node16 in the foreseeable future

Hacksore commented 1 year ago

I think it's pretty clear that @sindresorhus's packages are ESM only without any plans to support CJS.

Not sure the types will ever be accommodating for the crazy world of ESM v CJS but at least we have workarounds.

baptisteArno commented 1 year ago

Same... "moduleResolution": "Node16" introduces other issue in my next.js projects. For now, I have to revert to v12

throrin19 commented 1 year ago

Same with "moduleResolution": "node"

conioX commented 1 year ago

why we need support this esm :( most framework dont support esm

hugo082 commented 1 year ago

For those who wants to disable the warning, please refer to https://github.com/vercel/next.js/discussions/49432 and https://github.com/jaredwray/keyv/issues/45:

- warn Compiled with warnings

./node_modules/keyv/src/index.js
Critical dependency: the request of a dependency is an expression

Import trace for requested module:
./node_modules/keyv/src/index.js
./node_modules/cacheable-request/dist/index.js
./node_modules/got/dist/source/core/index.js
./node_modules/got/dist/source/index.js
./src/app/api/bug/route.ts

You can override the webpack configuration with:

/** @type {import('next').NextConfig} */
const nextConfig = {
  // ...
  webpack: (config) => {
    // Ignore ALL webpack warnings produced by ./node_modules/keyv/src/index.js file
    config.ignoreWarnings = [
      { module: /node_modules\/keyv\/src\/index\.js/ },
    ];

    return config;
  },
};

module.exports = nextConfig;

⚠️ Note that it will ignore ALL warnings produced by this file, not only the request of a dependency is an expression but you can improve the snippet to filter only this message, please refer to ignoreWarnings documentation

Tells webpack to ignore specific warnings. This can be done with a RegExp, a custom function to select warnings based on the raw warning instance which is getting WebpackError and Compilation as arguments and returns a boolean, an object with the following properties:

trim21 commented 1 year ago

why we need support this esm :( most framework dont support esm

why intentionally break "moduleResolution": "Node"

timeimp commented 1 year ago

Typescript 5.2 now explicity fails to build when you mix module and moduleResolutions in ways you're not meant to.

See https://github.com/microsoft/TypeScript/pull/54567

error TS5110: Option 'module' must be set to 'Node16' when option 'moduleResolution' is set to 'Node16'.

(or some variation of that).

This means anyone upgrading to 5.2 and using got 13.0.0 will suddenly hit this error.

Using the above from @datner, I managed to get a repo working but ran into another error.

The other error - ERR_UNSUPPORTED_DIR_IMPORT - had a simple fix.

I used this change to tsconfig.json:

"paths": {
    "got": ["./node_modules/got/dist/source/index.js"]
},

I figured I'd mention it here now that 5.2 is out and this may become a larger issue before the end of the year.

datner commented 1 year ago

@timeimp yeah if you use "module": "Node16" or "NodeNext" then typescript expects full and extensioned imports. No inferring index or skipping the .js

dandv commented 1 year ago

I also can't get got v13 to work with jest. I've tried all sorts of settings mentioned here and in #1952.

datner commented 1 year ago

@dandv add this to your jest config:

    /**
     * "Not a ts-jest issue" but a jest one. Consider vitest?
     *  this morphs "./some/relative/path.js" to "./some/relative/path"
     *  https://github.com/kulshekhar/ts-jest/issues/1057
     */
    moduleNameMapper: {
        "^(\\.\\.?\\/.+)\\.js$": "$1",
    },

Not sure thats exactly it, but I suspect this might help. Maybe.

dandv commented 1 year ago

@datner: that didn't work, unfortunately. Same error.

JoCat commented 1 year ago

Great, after updating got, syntax highlighting in VSCode doesn't work. Doesn't matter if it is used:

"module": "node16",
"moduleResolution": "node16"

image or not. image

throrin19 commented 1 year ago

To avoid problems I use the package wrapper got-cjs

kbzowski commented 1 year ago

To avoid problems I use the package wrapper got-cjs

But got-cjs is not updated. It is still 12. Dynamic import and type GotModule = typeof import('got'); worked fine with got 12, stopped with 13.

In my case yarn patch solved the problem.

dream2333 commented 1 year ago

same problem

bodrovis commented 1 year ago

Unfortunately certain customers that are using my SDK (that relies on Got) are also experiencing the same issue and it becomes a bit of a problem as it's not feasible for them to try all the solutions listed in this thread. I decided to give the Fetch API a go instead and it seems to do the trick

rdzidziguri commented 12 months ago

The package's author does not care much about making the changes needed to make it work again. :slightly_smiling_face: I am. We ended up getting blocked from upgrading. Now, the team suggests dropping the package in favor of other competitors as we have 500+ consumers (businesses), and nobody can upgrade, as the changes mentioned here are not feasible. I am so sorry to leave the package, as we had some great experiences with it till V13 was released. You might be surprised, but some companies run SOC 2 audits as well, and we are forced to keep the package up to date, so without having the option to make this backward compatible, we are going to switch to fat alternatives, unfortunately.

When the version you are trying to push does not gat the traction, it probably would be a good idea to think twice before killing userbase. image

micthiesen commented 12 months ago

Another workaround in case it's useful:

tsconfig.json

 "typeRoots": ["./node_modules/@types", "./types"]

types/got-fix.d.ts

declare module "got" {
  import got from "got/dist/source";
  export = got;
}
rdzidziguri commented 12 months ago

If you want to make it work and you are on the latest TS 5.2.2, you can also set moduleResolution to Bundler; however, this makes you use the cutting-edge stack, which is not always the case, especially for large projects with a significant legacy.

Denny966 commented 11 months ago

I couldn't get any of the above solutions working with my setup so I created my own type file with some of the got API functions I use.

It is by no means complete but it removes the warnings and it is strongly typed.

declare module "got" {

    interface GotResponse {
        json<U>(): Promise<U>;
        text(): Promise<string>;
    }

    export function get(url: string, options: Options): GotResponse;
    export function post(url: string, options?: Options): GotResponse;

    export interface Options {
        headers?: Record<string, string>;
        method?: "GET"| "POST";
        timeout?: {
            request: number
        },
        responseType?: "json" | "buffer" | "text";
        url?: string;
        body?: any;
        json?: boolean;
    }
}
pho3nixf1re commented 11 months ago

I extended @micthiesen 's solution to be a bit more direct. You shouldn't need to add it to typeRoots unless you have some kind of odd setup as .d.ts files will normally work out of the box within the project but... YMMV. Note, this is for a distributed CLI tool and not Next.js. I am unsure how this would work with Next.

File: /types/got.d.ts

declare module 'got' {
  export * from 'got/dist/source/index.ts';
}