Improve errors on module: node12 and extensionless relative imports #46486

gabritto · 2021-10-22T19:56:11Z

src/compiler/checker.ts

DanielRosenwasser · 2021-10-22T20:21:13Z

src/compiler/checker.ts

@@ -3503,6 +3518,18 @@ namespace ts {
                        hasJsonModuleEmitEnabled(compilerOptions)) {
                        error(errorNode, Diagnostics.Cannot_find_module_0_Consider_using_resolveJsonModule_to_import_module_with_json_extension, moduleReference);
                    }
+                    else if (isESMFile && resolutionIsNode12OrHigher && isExtensionlessRelativePathImport) {
+                        const absoluteRef =  getNormalizedAbsolutePath(moduleReference, getDirectoryPath(currentSourceFile.path));
+                        const suggestedExt = suggestedExtensions.find(([actualExt, _importExt]) => host.fileExists(absoluteRef + actualExt))?.[1];


This probes the FS - do we have any concerns there from a performance standpoint?

Generally the answer is “no” when we’re about to emit an error, but it is kind of notable that nowhere else in the checker uses fileExists. You could use host.getSourceFile instead which would (for all of our own TypeCheckerHost implementations) only see if there was a file by that name we already loaded for some reason. This will often be sufficient given a project’s default includes glob, but won’t work in general, so it kind of depends on whether we want this message to be a sure thing or a medium-effort heuristic. I kind of lean toward host.fileExists is already there, so it’s probably ok to use...

On the other hand, this error message could be a pretty hot path if you take a big codebase and flip it from --module commonjs to node12 before updating anything. But doing this work at some point seems unavoidable if we want to give the better error.

Is this something we can move into the program construction phase then? Do we ever build errors there?

If you're going to probe a bunch of paths in the same directory, it can be faster to use readdir and then compare against the results yourself.

There's some danger that people will see this thousands of times when they first change the setting, but I'm a little reluctant to "optimize" it without some way to establish that the change helped. Probably better to keep it simple until we measure a problem.

Is this something we can move into the program construction phase then? Do we ever build errors there?

After thinking about this more, I think maybe the ideal implementation would probe the filesystem during module resolution—when ESM-mode module resolution fails, it could try CJS-mode resolution and attach the result to the ResolvedModuleWithFailedLookupLocations result. The checker would then call getResolvedModuleWithFailedLookupLocationsFromCache (already on program but needs to be added to TypeCheckerHost) when it sees an unresolved import to check and see if CJS-mode resolution would have worked, and if so, exactly what file it would have resolved to.

It's probably worth noting that the fileExists check may not even be sufficient in the presence of export (or import) maps - if you wanna know if the specifier-with-some-specific-extension would have resolved, you pretty much need to rerun the whole resolution process now. Precaching some extra specifier resolutions could work, but it certainly wouldn't be free to do (especially given that all of .js, .cjs, and .mjs specifiers would need testing).

I think @andrewbranch's suggestion is a good one for suggesting extensions. I don't think I could get this ready for the RC, though. Does it still make sense to include this PR as is? I think the error message without the suggestion is going to remain as is, so that should go in. I think it would only make sense to remove the extension suggestion message/check if we have performance concerns over calling fileExists. Is that the case?

src/compiler/checker.ts

src/compiler/diagnosticMessages.json

DanielRosenwasser · 2021-10-22T23:59:25Z

tests/baselines/reference/nodeModulesAllowJs1(module=nodenext).errors.txt

@@ -64,27 +64,27 @@ tests/cases/conformance/node/allowJs/index.js(59,22): error TS1471: Module './su
 tests/cases/conformance/node/allowJs/index.js(60,1): error TS8002: 'import ... =' can only be used in TypeScript files.
 tests/cases/conformance/node/allowJs/index.js(60,22): error TS1471: Module './subfolder2/another/index' cannot be imported using this construct. The specifier only resolves to an ES module, which cannot be imported synchronously. Use dynamic import instead.
 tests/cases/conformance/node/allowJs/index.js(74,21): error TS2307: Cannot find module './' or its corresponding type declarations.
-tests/cases/conformance/node/allowJs/index.js(75,21): error TS2307: Cannot find module './index' or its corresponding type declarations.
-tests/cases/conformance/node/allowJs/index.js(76,21): error TS2307: Cannot find module './subfolder' or its corresponding type declarations.


This one is interesting - technically we'd want to tell people that they have to import from index.js. It's not a big deal.

My comment at #46486 (comment) would fix this. It’s probably not worth blocking over if we don’t have time to try this before the RC, but we should at least follow up in 4.6 to try it.

weswigham · 2021-10-27T20:51:16Z

tests/baselines/reference/nodeModulesAllowJs1(module=node12).errors.txt

-tests/cases/conformance/node/allowJs/index.cjs(84,21): error TS2307: Cannot find module './subfolder2/another/' or its corresponding type declarations.
-tests/cases/conformance/node/allowJs/index.cjs(85,21): error TS2307: Cannot find module './subfolder2/another/index' or its corresponding type declarations.
+tests/cases/conformance/node/allowJs/index.cjs(76,21): error TS2835: Relative import paths need explicit file extensions in ES modules when '--moduleResolution' is 'node12' or 'nodenext'. Did you mean './index.mjs'?
+tests/cases/conformance/node/allowJs/index.cjs(77,21): error TS2834: Relative import paths need explicit file extensions in ES modules when '--moduleResolution' is 'node12' or 'nodenext'.


The other common case is an import like this one - where a full index.js is potentially missing, and not just the extension. Might be worth specializing the error for that case, too.

src/compiler/checker.ts

gabritto · 2021-10-29T01:16:15Z

I added new test cases to cover every combination of import type + CJS/ESM file I could think of.
Possible import types: import statements, import =, dynamic imports, possible files: cts, mts.
There are also two extra tests for suggesting .jsx and .js extensions for a .tsx file.

Note: I still have to update the old test baselines to reflect the new error messages; CI will fail until then.

gabritto · 2021-10-29T01:19:45Z

tests/baselines/reference/moduleResolutionWithoutExtension6.errors.txt

+    // but other errors should not assume that they are allowed
+    import { foo } from "./foo"; // should error, should not ask for extension
+                        ~~~~~~~
+!!! error TS2307: Cannot find module './foo' or its corresponding type declarations.


For future reference, I think it's a bit weird that the error message here is TS2307 "Cannot find module", instead of something like TS1471: "Module '{0}' cannot be imported using this construct".

andrewbranch

I think during 4.6 we might try to replace this with some targeted fallback-to-CJS-resolution attempts in the module resolver itself, but this is a big improvement and is worth shipping in 4.5 to help users get a feel for how to use node12/nodenext.

…icrosoft#46486) * add new error + suggestions * push down assert defined * remove comment * fix esm module import detection, update baselines * add and update new tests * fix review comments * remove renamed baseline references * update node modules test baselines * fix error message, add new tests * update old tests with new error message

gabritto added 3 commits October 22, 2021 12:38

add new error + suggestions

8811903

push down assert defined

ffcaf66

remove comment

9c9899c

typescript-bot assigned gabritto Oct 22, 2021

typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Oct 22, 2021

gabritto commented Oct 22, 2021

View reviewed changes

src/compiler/checker.ts Show resolved Hide resolved

gabritto requested a review from DanielRosenwasser October 22, 2021 19:58

DanielRosenwasser reviewed Oct 22, 2021

View reviewed changes

src/compiler/checker.ts Outdated Show resolved Hide resolved

DanielRosenwasser reviewed Oct 22, 2021

View reviewed changes

src/compiler/checker.ts Show resolved Hide resolved

DanielRosenwasser reviewed Oct 22, 2021

View reviewed changes

src/compiler/checker.ts Outdated Show resolved Hide resolved

DanielRosenwasser reviewed Oct 22, 2021

View reviewed changes

andrewbranch reviewed Oct 22, 2021

View reviewed changes

src/compiler/checker.ts Show resolved Hide resolved

gabritto added 2 commits October 22, 2021 14:58

fix esm module import detection, update baselines

7166cbe

add and update new tests

998dbb1

amcasey reviewed Oct 22, 2021

View reviewed changes

src/compiler/diagnosticMessages.json Outdated Show resolved Hide resolved

DanielRosenwasser reviewed Oct 22, 2021

View reviewed changes

gabritto added 2 commits October 25, 2021 12:31

fix review comments

e8a1b7f

remove renamed baseline references

4e4a168

weswigham reviewed Oct 27, 2021

View reviewed changes

update node modules test baselines

685e98f

DanielRosenwasser reviewed Oct 28, 2021

View reviewed changes

src/compiler/checker.ts Show resolved Hide resolved

fix error message, add new tests

f89332e

gabritto requested review from DanielRosenwasser, andrewbranch and weswigham October 29, 2021 01:16

gabritto commented Oct 29, 2021

View reviewed changes

update old tests with new error message

4b0da27

andrewbranch approved these changes Oct 29, 2021

View reviewed changes

andrewbranch merged commit 9cdbb72 into main Oct 29, 2021

andrewbranch deleted the gabritto/issue46152 branch October 29, 2021 17:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve errors on module: node12 and extensionless relative imports #46486

Improve errors on module: node12 and extensionless relative imports #46486

gabritto commented Oct 22, 2021

DanielRosenwasser Oct 22, 2021

andrewbranch Oct 22, 2021

DanielRosenwasser Oct 22, 2021

amcasey Oct 22, 2021

amcasey Oct 22, 2021

andrewbranch Oct 27, 2021

weswigham Oct 27, 2021

gabritto Oct 27, 2021

DanielRosenwasser Oct 22, 2021

andrewbranch Oct 27, 2021

weswigham Oct 27, 2021

gabritto commented Oct 29, 2021

gabritto Oct 29, 2021

andrewbranch left a comment

Improve errors on module: node12 and extensionless relative imports #46486

Improve errors on module: node12 and extensionless relative imports #46486

Conversation

gabritto commented Oct 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabritto commented Oct 29, 2021

Choose a reason for hiding this comment

andrewbranch left a comment

Choose a reason for hiding this comment