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

When using require() in es modules, no TS2441 error occurs and node_modules/@types is automatically included in the project. #48726

Closed
jespertheend opened this issue Apr 15, 2022 · 9 comments
Assignees
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue Has Repro This issue has compiler-backed repros: https://aka.ms/ts-repros Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@jespertheend
Copy link
Contributor

jespertheend commented Apr 15, 2022

Bug Report

πŸ”Ž Search Terms

cjs require

πŸ•— Version & Regression Information

  • This changed between versions 4.0.5 and 4.2.3

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

// @filename: index.js
// @checkJs: true

/**
 * @param {string} str
 */
function require(str) {}

const someModule = require("google-closure-compiler");

export {someModule};

πŸ™ Actual behavior

TS2792 occurs at the call to require():

const someModule = require("google-closure-compiler");
//                         ^^^^^^^^^^^^^^^^^^^^^^^^^----- Cannot find module 'google-closure-compiler'. option?
//                                                        Did you mean to set the 'moduleResolution' option to 'node',
//                                                        or to add aliases to the 'paths'

What's worse, if the project contains types in node_modules/@types, these will now be included in the project, regardless of the value set for "typeRoots": [] in tsconfig.json, which is supposed to prevent this sort of behaviour.

From https://www.typescriptlang.org/tsconfig#typeRoots:

If typeRoots is specified, only packages under typeRoots will be included.

For me this caused a long and painful process of figuring out why I kept getting errors for imports missing from the "path" module, even though I definitely disabled node imports.

Here you can download a reduced test case of the situation described above.

Run tsc --noEmit -p jsconfig.json in the folder and you'll get an error stating that fromFileUrl is not in the "path" module, even though node_modules/@types has been disabled and a custom path has been added in jsconfig.json.

πŸ™‚ Expected behavior

Either no error, or an error informing me that I can't use require() as it's already taken by the compiler. The latter is already the case with TypeScript files:

function require(str: string) {}
//       ^^^^^^^------ TS2441: Duplicate identifier 'require'.
//                     Compiler reserves name 'require' in top level scope of a module.

It's just missing inside JavaScript files.

πŸ”ƒ Workarounds

Just don't name your functions require πŸ˜…. But you'd have to know that this issue is happening in the first place, which is not easy to figure out with no errors telling you this.

@andrewbranch andrewbranch added Bug A bug in TypeScript Has Repro This issue has compiler-backed repros: https://aka.ms/ts-repros labels Apr 21, 2022
@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 21, 2022

πŸ‘‹ Hi, I'm the Repro bot. I can help narrow down and track compiler bugs across releases! This comment reflects the current state of the repro in the issue body running against the nightly TypeScript.


Issue body code block by @jespertheend

❌ Failed: -

  • Cannot find module 'google-closure-compiler' or its corresponding type declarations.

Historical Information
Version Reproduction Outputs
4.2.2, 4.3.2, 4.4.2, 4.5.2, 4.6.2

❌ Failed: -

  • Parameter 'str' implicitly has an 'any' type.

@andrewbranch
Copy link
Member

@typescript-bot bisect good v4.0.5 bad v4.1.5

@typescript-bot
Copy link
Collaborator

The change between v4.0.5 and v4.1.5 occurred at c3d41bb.

@andrewbranch andrewbranch added this to the TypeScript 4.8.0 milestone Apr 21, 2022
@typescript-bot typescript-bot added the Fix Available A PR has been opened for this issue label Aug 1, 2022
@sandersn
Copy link
Member

sandersn commented Aug 2, 2022

After the discussion on #50130, I think the right fix is to show an error; require calls in JS are almost always meant to import something even if the environment isn't strict commonjs.

However, the current error is emit-oriented; it'll show up in JS or TS files if your tsconfig targets es5, but that is not common, for the JS projects I've written at least. The error needs to show up for any JS file instead.

@jespertheend
Copy link
Contributor Author

It's been a while since I opened this, so the exact details are a bit fuzzy. But fwiw my use case was that I'm using deno for running and tsc for type checking. Deno std has a function for importing cjs modules:

import { createRequire } from "https://deno.land/std@0.150.0/node/module.ts";
const require = createRequire(import.meta.url);
const closureCompiler = require("google-closure-compiler");

I worked around the issue by renaming require, so my issue has been solved.
But I spent a good amount of time scratching my head as to why the typedefs from node_modules/@types were being included, even though I had definitely set typeRoots in tsconfig.json to an empty array.

An error at const require = would certainly be helpful in this case.

@sandersn
Copy link
Member

sandersn commented Aug 2, 2022

Interesting. It looks like the ideal would be for typescript to be able to resolve google-closure-compiler and not issue an error on require, since it's intended as a function that imports things. Not sure how practical that would be given your setup.

@jespertheend
Copy link
Contributor Author

I reckon my setup is highly unusual xD. I don't think anyone else uses tsc for checking files intended for running with Deno.
I think the Deno language server normally takes care of this.

@sandersn
Copy link
Member

sandersn commented Aug 3, 2022

I think after all that I'm inclined to say that the current behaviour is good for most cases, and that the usual desired outcome would be for require to resolve google-closure-compiler though manipulating the environment.

@sandersn sandersn closed this as completed Aug 3, 2022
@sandersn sandersn added the Working as Intended The behavior described is the intended behavior; this is not a bug label Aug 3, 2022
@andrewbranch
Copy link
Member

One of the things I softly pitched in #50152 was a module resolution mode in which require would have no special meaning even in JS files. In TS files compiled to CommonJS, people sometimes use require as an escape hatch for requiring libraries or paths whose types are messed up or missing, or for requiring files with extensions that TypeScript doesn’t understand. In general, I don’t see why JS files that are known to be ES modules would behave any differently. However, the createRequire case does sort of throw a wrench in thatβ€”it’s pretty magical that it works, though perhaps by accident. Anyway, I might revisit this down the road if it interacts with whatever we decide to do in #50152.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue Has Repro This issue has compiler-backed repros: https://aka.ms/ts-repros Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
4 participants