`--moduleResolution bundler`: Require ESM for `module` and remove `node` from hard-coded conditions #52940

andrewbranch · 2023-02-23T19:30:39Z

Also fixes some missing traces and makes some minor error message tweaks.

…is bundler

andrewbranch · 2023-02-23T19:35:53Z

src/compiler/checker.ts

            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;
            }


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.

andrewbranch · 2023-02-23T19:37:43Z

src/compiler/checker.ts

-                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);
-                }
            }


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.

andrewbranch · 2023-02-23T19:39:51Z

src/compiler/moduleNameResolver.ts

    const conditions = esmMode || getEmitModuleResolutionKind(options) === ModuleResolutionKind.Bundler
-        ? ["node", "import"]
-        : ["node", "require"];
+        ? ["import"]
+        : ["require"];
+    if (getEmitModuleResolutionKind(options) !== ModuleResolutionKind.Bundler) {
+        conditions.unshift("node");
+    }


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.

Small nit suggestion: Instead of unshift can we first add "node" and then others so we arent moving array elements

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.

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.

andrewbranch · 2023-02-23T19:41:58Z

...reference/bundlerImportTsExtensions(allowimportingtsextensions=true,noemit=false).errors.txt

@@ -83,5 +83,5 @@ error TS6054: File '/project/e.txt' has an unsupported extension. The only suppo
    import {} from "./a.ts";
    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?


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.

Andarist · 2023-02-23T19:48:24Z

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)

andrewbranch · 2023-02-23T19:52:48Z

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.

Andarist · 2023-02-24T11:02:26Z

Do all bundlers always set the module condition? Even if this is true today, I wouldn’t want to count on it.

Definitely not all of them. I know that it's supported by:

webpack5
the Rollup versions that understand conditions
esbuild (only in recent versions, previously you could enable it just like any other condition)
bun

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)

andrewbranch · 2023-02-24T16:35:35Z

--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.

andrewbranch · 2023-02-24T16:37:28Z

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.

sheetalkamat · 2023-02-24T20:04:44Z

src/compiler/moduleNameResolver.ts

    const conditions = esmMode || getEmitModuleResolutionKind(options) === ModuleResolutionKind.Bundler
-        ? ["node", "import"]
-        : ["node", "require"];
+        ? ["import"]
+        : ["require"];
+    if (getEmitModuleResolutionKind(options) !== ModuleResolutionKind.Bundler) {
+        conditions.unshift("node");
+    }


Small nit suggestion: Instead of unshift can we first add "node" and then others so we arent moving array elements

andrewbranch added 2 commits February 23, 2023 11:00

Tweak errors and make bundler require ES modules

e5c8c97

Remove node condition from always-included set when moduleResolution …

dd5a89d

…is bundler

typescript-bot assigned andrewbranch Feb 23, 2023

typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Feb 23, 2023

andrewbranch commented Feb 23, 2023

View reviewed changes

jakebailey approved these changes Feb 24, 2023

View reviewed changes

sheetalkamat approved these changes Feb 24, 2023

View reviewed changes

Change conditions order to avoid array unshift

f234960

andrewbranch merged commit e9868e9 into microsoft:main Feb 24, 2023

andrewbranch deleted the bundler-updates branch February 24, 2023 23:49

andrewbranch mentioned this pull request Jun 22, 2023

--moduleResolution bundler (formerly known as hybrid) #51669

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`--moduleResolution bundler`: Require ESM for `module` and remove `node` from hard-coded conditions #52940

`--moduleResolution bundler`: Require ESM for `module` and remove `node` from hard-coded conditions #52940

andrewbranch commented Feb 23, 2023

andrewbranch Feb 23, 2023

andrewbranch Feb 23, 2023

andrewbranch Feb 23, 2023

sheetalkamat Feb 24, 2023

andrewbranch Feb 24, 2023

sheetalkamat Feb 25, 2023

andrewbranch Feb 23, 2023

Andarist commented Feb 23, 2023

andrewbranch commented Feb 23, 2023

Andarist commented Feb 24, 2023

andrewbranch commented Feb 24, 2023

andrewbranch commented Feb 24, 2023

sheetalkamat Feb 24, 2023

--moduleResolution bundler: Require ESM for module and remove node from hard-coded conditions #52940

--moduleResolution bundler: Require ESM for module and remove node from hard-coded conditions #52940

Conversation

andrewbranch commented Feb 23, 2023

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

Andarist commented Feb 23, 2023

andrewbranch commented Feb 23, 2023

Andarist commented Feb 24, 2023

andrewbranch commented Feb 24, 2023

andrewbranch commented Feb 24, 2023

Choose a reason for hiding this comment

`--moduleResolution bundler`: Require ESM for `module` and remove `node` from hard-coded conditions #52940

`--moduleResolution bundler`: Require ESM for `module` and remove `node` from hard-coded conditions #52940