4.9.3 commonjs output regression: Dynamical path import()s inside sync for-of are all called with the same parameter

[thgreasi]

Bug Report

🔎 Search Terms

4.9, compiler regression, for-of, import(), commonJS

🕗 Version & Regression Information

Works fine in 4.8.4 & 4.9.1-beta.
First saw this in the 4.9.2-rc & 4.9.3 and still exists in the current nightly 5.0.0-dev.20221116.

Please keep and fill in the line that best applies:
-->

• This changed between versions 4.9.1-beta and 4.9.2-rc

⏯ Playground Link

v4.9.3 commonjs output with the issue

Properly working v4.8.4 commonjs output

💻 Code

Compiling the following while targeting es2022 w/ commonjs output.

for (const file of [1,2,3]) {
	import(`./${file}.json`);
}

PS: The actual code I'm using is storing the resulting promises, but didn't include it in the minimum reproduction code since it didn't make any difference.

🙁 Actual behavior

It imports file 3.json three times, b/c of a new var a_ outside the for-of scope is created and re-used across iterations.

var _a; // <-- was not emitted before 4.9.2-rc
for (const model of [1, 2, 3]) {
    _a = `./${model}.json`, Promise.resolve().then(() => __importStar(require(_a)));
}

🙂 Expected behavior

It should import files 1.json, 2.json & 3.json, just like up to 4.9.1-beta.

[Josh-Cena]

This is caused by #49663

Maybe it's better to emit Promise.resolve(`${arg}`).then(c => require(c)) as mentioned in #49663 (comment) after all?

[fatcerberus]

Problem is that that introduces an additional event loop tick vs. a native dynamic import, which might be observable at runtime and is probably a spec violation.

Seems like the issue is that it's putting the temp variable in the outer scope for some reason. It would be better to emit a let in the same scope, which would work for any target later than es5.

[Josh-Cena]

@fatcerberus We currently emit Promise.resolve().then(), which is exactly the same as putting something within the resolve(). Spec doesn't say a module—even a cached one—must be instantly fulfilled. See HostImportModuleDynamically:

At some future time, the host environment must perform FinishDynamicImport(referencingScriptOrModule, specifier, promiseCapability, promise), where promise is a Promise resolved with undefined.

(Next bullet talks about no double evaluation, not how import() should be fulfilled)

My proposal is also exactly how babel-plugin-dynamic-import-node does it, so we are no worse than Babel.

[simllll]

Just ran into the same issue, this is a massive bummer for ts4.9! And quite hard to find.