Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Improve errors on module: node12 and extensionless relative imports #46486
Improve errors on module: node12 and extensionless relative imports #46486
Changes from all commits
8811903
ffcaf66
9c9899c
7166cbe
998dbb1
e8a1b7f
4e4a168
685e98f
f89332e
4b0da27
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probes the FS - do we have any concerns there from a performance standpoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 usehost.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 defaultincludes
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 towardhost.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
tonode12
before updating anything. But doing this work at some point seems unavoidable if we want to give the better error.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something we can move into the program construction phase then? Do we ever build errors there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 callgetResolvedModuleWithFailedLookupLocationsFromCache
(already on program but needs to be added toTypeCheckerHost
) 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".