Update error messages for CJS imports resolving to ES modules #50088
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
| @@ -191,10 +191,10 @@ tests/cases/conformance/node/index.ts(84,21): error TS2835: Relative import path | |||
| // These should _mostly_ work - `import = require` always desugars to require calls, which do have extension and index resolution (but can't load anything that resolves to esm!) | |||
| import m24 = require("./"); | |||
| ~~~~ | |||
| !!! error TS1471: Module './' cannot be imported using this construct. The specifier only resolves to an ES module, which cannot be imported synchronously. Use dynamic import instead. | |||
| !!! error TS1471: Module './' cannot be imported using this construct. The specifier only resolves to an ES module, which cannot be imported with 'require'. Use an ECMAScript import instead. | |||
There was a problem hiding this comment.
It took me a bit to convince myself that this worked because I forgot that we allowed import = require in ES modules.
There was a problem hiding this comment.
Prediction: no user will ever see this error message
There was a problem hiding this comment.
I still don't think recommending a dynamic import is very useful, but I think the related info is the big winner here.
I am surprised that we had no(?) tests for an import of an ES module from a .ts file when type: module is unspecified. The related diagnostic only shows up in new tests... But glad we have them now!
Otherwise, I think this is good to go. It's strictly an improvement, and I'll leave it to your judgment whether we want to special-case the no-package.json case.
…dits (#50098) * Add test where module resolution cache is not local and hence doesnt report errors in watch mode * Ensure module resolution cache is passed through in watch mode * Remove unnecessary setting of impliedFormat which should anyways be done as part of create source file * Add test for packge.json changing and modifying implied format * Distinguish between package.json watch and affecting file location watch * Pass in failed lookup and affected file locations for source file's implied format Also stop creating options if we already have them * Add diagnostic for explaining file's implied format if based on package.json * Watch implied format dependencies for modules and schedule update on change * For program if implied node format doesnt match create new source file. Handle implied node format in document registry Fixes #50086 * Modify tests to show package.json being watched irrespective of folder its in * Check file path if it can be watched before watching package.json file * Because we are watching package.json files and failed lookups its safe to invalidate package json entries instead of clearing them out everytime program is created * Remove todos * Fix the incorrect merge * Pickup PackageJsonInfo renames from #50088 * Rename
|
@sheetalkamat do I still need to cherry-pick some commits into this or did you get those fixes into main? |
| [7m [0m [96m ~~~~~~[0m | ||
| To convert this file to an ECMAScript module, change its file extension to '.mts' or create a local package.json file with `{ "type": "module" }`. | ||
|
|
||
| [[90m12:01:16 AM[0m] Found 6 errors. Watching for file changes. |
There was a problem hiding this comment.
@sheetalkamat this error message seem to be duplicated in tsbuildWatch and tsserver unit tests and I have no idea why—any ideas?
There was a problem hiding this comment.
To me it seems like for some reason they are not getting de-dupped?
There was a problem hiding this comment.
It seems like errors with related info don’t get deduped because the deduping logic tends to happen before attaching the related info, so the diagnostic being added doesn’t appear to be equal to the ones already in the collection yet. I switched to a message chain since related info is better suited for pointing to a different location, but we should maybe look and see if this is happening elsewhere.
|
I will probably out when Sheetal has a chance to review again—feel free to merge if it looks ok. |
Fixes #50009