Return rejected promise when stringify import specifier throws

[SuperSodaSea]

Q	A
Fixed Issues?	Fixes #15261
Patch: Bug Fix?	👍
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?
License	MIT

When using dynamic import (import(specifier)), the toString of the specifier should be evaluated immediately, not in promise. And if the toString throws, import() should return a rejected promise instead of throwing it immediately.

See #15261 (comment) for more context.

Tests

➕ Added

• babel-plugin-proposal-dynamic-import
  • amd/to-string
  • commonjs/exec-throw
  • commonjs/exec-to-primitive
  • commonjs/exec-to-string-order
  • commonjs/exec-to-string-throw
  • commonjs/template-literal
  • systemjs/to-string

〰️ Output Changed

• babel-plugin-proposal-dynamic-import
  • commonjs/to-string

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/53692/

[JLHwung]

LGTM except for the toPrimitive cases.

[JLHwung]

Suggested change

	new Promise(r => r("" + source))
	new Promise(r => r(${
	t.isTemplateLiteral(source) ? source : t.templateLiteral([], [source])
	}))

The transform from ${x} to "" + x relies on the ignoreToPrimitiveHint assumption.

Consider an exotic object:

var source = {
  [Symbol.toPrimitive](hint) {
    if (hint === 'string') {
      return "foo"
    }
    return null
  }
}

"" + source will be null while ${source} will be "foo". The ToString operation should respect the toPrimitive symbol.

[SuperSodaSea]

Fixed, and I go further to move the common logic to function buildDynamicImport:

babel/packages/babel-helper-module-transforms/src/dynamic-import.ts

Lines 18 to 23 in 9b21005

	export function buildDynamicImport(
	node: t.CallExpression,
	deferToThen: boolean,
	wrapWithPromise: boolean,
	builder: (specifier: t.Expression) => t.Expression,
	): t.Expression {

And in transform-modules-amd/commonjs/system we only need to write code once:

babel/packages/babel-plugin-transform-modules-amd/src/index.ts

Lines 102 to 115 in 9b21005

	buildDynamicImport(
	path.node,
	false,
	false,
	specifier => template.expression.ast`
	new Promise((${resolveId}, ${rejectId}) =>
	${requireId}(
	[${specifier}],
	imported => ${t.cloneNode(resolveId)}(${result}),
	${t.cloneNode(rejectId)}
	)
	)
	`,
	),

babel/packages/babel-plugin-transform-modules-commonjs/src/dynamic-import.ts

Lines 24 to 26 in 9b21005

	buildDynamicImport(path.node, true, false, specifier =>
	buildRequire(specifier, file),
	),

babel/packages/babel-plugin-transform-modules-systemjs/src/index.ts

Lines 277 to 285 in 9b21005

	buildDynamicImport(path.node, false, true, specifier =>
	t.callExpression(
	t.memberExpression(
	t.identifier(state.contextIdent),
	t.identifier("import"),
	),
	[specifier],
	),
	),

[nicolo-ribaudo]

Since the AMD require function is already asynchronous, we don't need to defer it to .then — the original code was fine. Same for SystemJS.

[SuperSodaSea]

Ok, I just need to set the buildDynamicImport's runInThendeferToThen to false for it.

babel/packages/babel-plugin-transform-modules-amd/src/index.ts

Lines 102 to 115 in 9b21005

	buildDynamicImport(
	path.node,
	false,
	false,
	specifier => template.expression.ast`
	new Promise((${resolveId}, ${rejectId}) =>
	${requireId}(
	[${specifier}],
	imported => ${t.cloneNode(resolveId)}(${result}),
	${t.cloneNode(rejectId)}
	)
	)
	`,
	),

[SuperSodaSea]

Now the logic looks like this:

CommonJS (Deferred to then):

babel/packages/babel-helper-module-transforms/src/dynamic-import.ts

Lines 44 to 50 in 9b21005

	if (deferToThen) {
	return template.expression.ast`
	(specifier =>
	new Promise(r => r(${specifierToString}))
	.then(s => ${builder(t.identifier("s"))})
	)(${specifier})
	`;

SystemJS (Needs to return a rejected promise when toString throws so wrap it in a promise):

babel/packages/babel-helper-module-transforms/src/dynamic-import.ts

Lines 51 to 56 in 9b21005

	} else if (wrapWithPromise) {
	return template.expression.ast`
	(specifier =>
	new Promise(r => r(${builder(specifierToString)}))
	)(${specifier})
	`;

AMD (Builder already returns a new Promise so do nothing):

babel/packages/babel-helper-module-transforms/src/dynamic-import.ts

Lines 57 to 60 in 9b21005

	} else {
	return template.expression.ast`
	(specifier => ${builder(specifierToString)})(${specifier})
	`;

[nicolo-ribaudo]

Thank you!