--moduleResolution bundler: Require ESM for module and remove node from hard-coded conditions - #52940
Conversation
Sorry, something went wrong.
| if (emitModuleKindIsNonNodeESM(moduleKind) || mode === ModuleKind.ESNext) { | ||
| return importSourceWithoutExtension + (tsExtension === Extension.Mts ? ".mjs" : tsExtension === Extension.Cts ? ".cjs" : ".js"); | ||
| const preferTs = isDeclarationFileName(moduleReference) && shouldAllowImportingTsExtension(compilerOptions); | ||
| const ext = | ||
| tsExtension === Extension.Mts || tsExtension === Extension.Dmts ? preferTs ? ".mts" : ".mjs" : | ||
| tsExtension === Extension.Cts || tsExtension === Extension.Dmts ? preferTs ? ".cts" : ".cjs" : | ||
| preferTs ? ".ts" : ".js"; | ||
| return importSourceWithoutExtension + ext; | ||
| } |
There was a problem hiding this comment.
It has never made a ton of sense to predicate adding an extension to the suggested import source based on module and not moduleResolution, but rather than rethink that whole issue, I just made a narrow fix for when .ts is allowed and intended but the message suggests .js.
Sorry, something went wrong.
| } | ||
| else if (!(node.flags & NodeFlags.Ambient) && getEmitModuleResolutionKind(compilerOptions) === ModuleResolutionKind.Bundler) { | ||
| grammarErrorOnNode(node, Diagnostics.Import_assignment_is_not_allowed_when_moduleResolution_is_set_to_bundler_Consider_using_import_Asterisk_as_ns_from_mod_import_a_from_mod_import_d_from_mod_or_another_module_format_instead); | ||
| } | ||
| } |
There was a problem hiding this comment.
We can drop this error because it’s redundant with the one that already existed for using import= in --module esnext. That makes me feel even more like this is a coherent move.
Sorry, something went wrong.
| const conditions = esmMode || getEmitModuleResolutionKind(options) === ModuleResolutionKind.Bundler | ||
| ? ["node", "import"] | ||
| : ["node", "require"]; | ||
| ? ["import"] | ||
| : ["require"]; | ||
| if (getEmitModuleResolutionKind(options) !== ModuleResolutionKind.Bundler) { | ||
| conditions.unshift("node"); | ||
| } |
There was a problem hiding this comment.
Someone suggested dropping node from the hard-coded set of conditions when moduleResolution is bundler and I think that makes sense. I didn’t do it at first because Webpack includes the node condition, oddly enough, but esbuild doesn’t unless its target is set to Node. It definitely makes more sense to leave it up to customConditions.
Sorry, something went wrong.
There was a problem hiding this comment.
Small nit suggestion: Instead of unshift can we first add "node" and then others so we arent moving array elements
Sorry, something went wrong.
There was a problem hiding this comment.
I don't think the order matters; I just didn’t want to change a bunch of baselines unnecessarily. I did unshift because I remember Ron saying something about if you make an empty array and then push a non-number into it, that does a deopt. Unshift is probably worse though... I think I’ll just push node on last and change some traces.
Sorry, something went wrong.
There was a problem hiding this comment.
Interestingly conditions can be a Set|undefined i think.. we always check for contains(array, key) so may be set is better.. But thats not important for this PR.. just mentioning.
Sorry, something went wrong.
| import {} from "./a.d.ts"; | ||
| ~~~~~~~~~~ | ||
| !!! error TS2846: A declaration file cannot be imported without 'import type'. Did you mean to import an implementation file './a' instead? | ||
| !!! error TS2846: A declaration file cannot be imported without 'import type'. Did you mean to import an implementation file './a.ts' instead? |
There was a problem hiding this comment.
This is the error message that changed based on my small addition in the checker. It was suggesting ./a.js before, which is ok, but looks a bit weird when the user has explicitly opted into allowing .ts extensions.
Sorry, something went wrong.
|
Out of curiosity - did you consider any special handling for the module condition? It has some special meaning in bundlers - both ESM imports and CJS requires are allowed to load it (even though its code is authored in ESM) |
Sorry, something went wrong.
|
I didn’t, but because we lack the ability to remove conditions, I feel like the best move is to be conservative. Do all bundlers always set the module condition? Even if this is true today, I wouldn’t want to count on it. |
Sorry, something went wrong.
Definitely not all of them. I know that it's supported by:
I can't say - without further testing - that it works exactly in the same way in all of them. But generally speaking most bundlers are more permissive when it comes to mixing CJS and ESM, so when that's enabled I would expect all of them to allow requiring a source that is written in ESM. This is the very goal of this condition - bundlers didn't want to not have any escape hatch from the node's strict rules. For that reason, I ship the module condition in all libs that I maintain (or at least in those that target browsers) |
Sorry, something went wrong.
|
--moduleResolution bundler doesn’t check requires at all. But I can tell you from testing many bundlers that there is no coherent pattern across most bundlers as to whether using require in a given file is allowed. |
Sorry, something went wrong.
|
This is totally unrelated to the default set of conditions though. Conditions cannot affect how their target module kind is interpreted. An import condition pointing to a CJS module is a violation of convention only. |
Sorry, something went wrong.
| const conditions = esmMode || getEmitModuleResolutionKind(options) === ModuleResolutionKind.Bundler | ||
| ? ["node", "import"] | ||
| : ["node", "require"]; | ||
| ? ["import"] | ||
| : ["require"]; | ||
| if (getEmitModuleResolutionKind(options) !== ModuleResolutionKind.Bundler) { | ||
| conditions.unshift("node"); | ||
| } |
There was a problem hiding this comment.
Small nit suggestion: Instead of unshift can we first add "node" and then others so we arent moving array elements
Sorry, something went wrong.
Discussion: #52926, #51669 (comment)
Also fixes some missing traces and makes some minor error message tweaks.