only search require calls in JS files

[ajafff]

require('foo') in TypeScript files has no special meaning. Therefore we don't need to look it up and treat it like an import. It used to be in the same function as the lookup of dynamic imports to find both in one full AST walk.

• removed useless parameters as the SourceFile was already in scope
• split lookup function into two separate functions
• only look for require calls in JS files
• only look at JSDoc when searching ImportTypeNode in JS files (require will never be in JSDoc anyway)
• slightly optimized the regex while keeping it readable

Followup on #28104 /cc @sandersn

[ajafff]

This change uncovered some pretty surprising behavior: if you have require('something') in your code (it could even be a local function named require), it would resolve that module like a regular import and include its declaration file into the program.
For the same reason @types/node was implicitly included in the compilation of src/compiler. After this change I had to explicitly reference that declaration file to make it compile again.

[sandersn]

@DanielRosenwasser and I had a long discussion about @types/node, since this incorrect behaviour has existed since April 2017 [1]. I think it's OK to take this fix as long as we note that it's a breaking change and improve the "Are you missing node" error.

Specifically,
Change "Try npm i @types/node" to
"Try npm i @types/node and then add node to the types field in your tsconfig."

Adding @DanielRosenwasser to the review for signoff on the error message change.

[1] Note that the behaviour has existed for longer in JS, but is probably a good idea there.

[sandersn]

There's also a number of new "Cannot find module 'x'" messages in the prettier user test. Mind checking those out? jake runtests ru=user t=prettier, and then you can open the project in $ts/tests/cases/user/prettier/prettier

[ajafff]

There's also a number of new "Cannot find module 'x'" messages in the prettier user test.

There are baseline changes, but they are the same in this branch an in master.

I think it's OK to take this fix as long as we note that it's a breaking change and improve the "Are you missing node" error.

I agree the proposed error message makes more sense, but I don't think it's strictly related to this PR. There are a few more error messages suggesting installing @types/jquery, @types/jest or @types/mocha. Should they be changed, too?

Note that this change will not only break code that implicitly relies on @types/node. It will break compilation of TypeScript files that contain require('foo') where 'foo' resolves to a declaration file containing declarations of ambient modules or global variables which are used in the program without being explicitly referenced (through import, types in tsconfig or a type reference directive).

[sheetalkamat]

I think we can still keep this a single pass by passing regexp instead.

[ajafff]

It's possible, but IMO not very useful. It would save around 4 lines of code just to add new code to get determine the correct regexp to use. It also adds complexity by moving the regexp out of the function where its matches are used.
I doubt having a single pass of a complex regexp outperforms two passes with a simple regex each (also considering potential inlining).

[sheetalkamat]

It will save pass I think.

[ajafff]

Like I already said: I don't think there will be a noticeable difference in performance because of optimizations in the VM (like inlining). It's not like we are walking the whole AST twice.

[RyanCavanaugh]

What's the observable breaking change here?

[ajafff]

What's the observable breaking change here?

Having types: [] in tsconfig.json would normally prevent non-module declaration files from introducing any global augmentations.
But if you had require('foo') or a JSDoc import import('foo') in a TypeScript file, that was resolved to @types/foo, these declaration files were added to the program and thus can augment the global namespace.

This occurred to me in this PR because there is require('fs') which caused @types/node to be loaded. This implicitly introduced require, module and all those other global declarations.
This was counterintuitive as require and JSDoc import-types shouldn't have any special meaning in TS files.

[sheetalkamat]

Dont think its worth taking this at point given people might be depending on this behavior by now.