Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Return rejected promise when stringify import specifier throws #15290

Merged
merged 6 commits into from Dec 23, 2022
Merged

Return rejected promise when stringify import specifier throws #15290

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
00446f1
Improve dynamic import spec compliance
SuperSodaSea Dec 19, 2022
fe63c95
Evaluate source outside
SuperSodaSea Dec 20, 2022
75996fe
Add buildDynamicImport
SuperSodaSea Dec 21, 2022
aef26e9
AMD's require don't need defer
SuperSodaSea Dec 21, 2022
083ab30
Update buildDynamicImport
SuperSodaSea Dec 21, 2022
9b21005
Update buildDynamicImport
SuperSodaSea Dec 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/babel-helper-module-transforms/src/dynamic-import.ts
View file
Open in desktop
Expand Up @@ -4,6 +4,7 @@
import * as t from "@babel/types";
import template from "@babel/template";

// TODO(Babel 8): Remove this
SuperSodaSea marked this conversation as resolved.
Show resolved Hide resolved
export function getDynamicImportSource(
node: t.CallExpression,
): t.StringLiteral | t.TemplateLiteral {
SuperSodaSea marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
2 changes: 1 addition & 1 deletion packages/babel-plugin-proposal-dynamic-import/test/fixtures/amd/to-string/output.js
View file
Open in desktop
@@ -1,3 +1,3 @@
define(["require"], function (_require) {
new Promise(r => r(`${2}`)).then(s => new Promise((_resolve, _reject) => _require([s], imported => _resolve(babelHelpers.interopRequireWildcard(imported)), _reject)));
(source => new Promise(r => r("" + source)).then(s => new Promise((_resolve, _reject) => _require([s], imported => _resolve(babelHelpers.interopRequireWildcard(imported)), _reject))))(2);
});
11 changes: 11 additions & 0 deletions packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-throw/exec.js
View file
Open in desktop
@@ -0,0 +1,11 @@
expect(() => {
function f() { throw "should throw"; }
import(f());
}).toThrow("should throw");

expect(() => {
const a = {
get x() { throw "should throw"; },
};
import(a.x);
}).toThrow("should throw");
2 changes: 1 addition & 1 deletion packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/to-string/output.js
View file
Open in desktop
@@ -1 +1 @@
new Promise(r => r(`${2}`)).then(s => babelHelpers.interopRequireWildcard(require(s)));
(source => new Promise(r => r("" + source)).then(s => babelHelpers.interopRequireWildcard(require(s))))(2);
2 changes: 1 addition & 1 deletion packages/babel-plugin-proposal-dynamic-import/test/fixtures/systemjs/to-string/output.js
View file
Open in desktop
Expand Up @@ -4,7 +4,7 @@ System.register([], function (_export, _context) {
return {
setters: [],
execute: function () {
new Promise(r => r(`${2}`)).then(s => _context.import(s));
(source => new Promise(r => r("" + source)).then(s => _context.import(s)))(2);
}
};
});
21 changes: 11 additions & 10 deletions packages/babel-plugin-transform-modules-amd/src/index.ts
View file
Open in desktop
Expand Up @@ -9,7 +9,6 @@ import {
ensureStatementsHoisted,
wrapInterop,
getModuleName,
getDynamicImportSource,
} from "@babel/helper-module-transforms";
import { template, types as t } from "@babel/core";
import type { PluginOptions } from "@babel/helper-module-transforms";
Expand Down Expand Up @@ -98,7 +97,7 @@ export default declare<State>((api, options: Options) => {
let result: t.Node = t.identifier("imported");
if (!noInterop) result = wrapInterop(path, result, "namespace");

const source = getDynamicImportSource(path.node);
const [source] = path.node.arguments;

path.replaceWith(
t.isStringLiteral(source) ||
Expand All @@ -114,14 +113,16 @@ export default declare<State>((api, options: Options) => {
))
`
: template.expression.ast`
new Promise(r => r(${source}))
.then(s => new Promise((${resolveId}, ${rejectId}) =>
${requireId}(
[s],
imported => ${t.cloneNode(resolveId)}(${result}),
${t.cloneNode(rejectId)}
)
))
(source =>
new Promise(r => r("" + source))
.then(s => new Promise((${resolveId}, ${rejectId}) =>
${requireId}(
[s],
imported => ${t.cloneNode(resolveId)}(${result}),
${t.cloneNode(rejectId)}
)
))
)(${source})
`,
);
},
Expand Down
9 changes: 5 additions & 4 deletions packages/babel-plugin-transform-modules-commonjs/src/dynamic-import.ts
View file
Open in desktop
Expand Up @@ -3,7 +3,6 @@

import type { NodePath } from "@babel/traverse";
import { types as t, template, type File } from "@babel/core";
import { getDynamicImportSource } from "@babel/helper-module-transforms";

const requireNoInterop = (source: t.Expression) =>
template.expression.ast`require(${source})`;
Expand All @@ -20,7 +19,7 @@ export function transformDynamicImport(
) {
const buildRequire = noInterop ? requireNoInterop : requireInterop;

const source = getDynamicImportSource(path.node);
const [source] = path.node.arguments;

const replacement =
t.isStringLiteral(source) ||
Expand All @@ -29,8 +28,10 @@ export function transformDynamicImport(
Promise.resolve().then(() => ${buildRequire(source, file)})
`
: template.expression.ast`
new Promise(r => r(${source}))
.then(s => ${buildRequire(t.identifier("s"), file)})
(source =>
new Promise(r => r("" + source))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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],
),
),

.then(s => ${buildRequire(t.identifier("s"), file)})
)(${source})
`;

path.replaceWith(replacement);
Expand Down
14 changes: 6 additions & 8 deletions packages/babel-plugin-transform-modules-systemjs/src/index.ts
View file
Open in desktop
@@ -1,11 +1,7 @@
import { declare } from "@babel/helper-plugin-utils";
import hoistVariables from "@babel/helper-hoist-variables";
import { template, types as t } from "@babel/core";
import {
rewriteThis,
getModuleName,
getDynamicImportSource,
} from "@babel/helper-module-transforms";
import { rewriteThis, getModuleName } from "@babel/helper-module-transforms";
import type { PluginOptions } from "@babel/helper-module-transforms";
import { isIdentifierName } from "@babel/helper-validator-identifier";
import type { NodePath, Scope, Visitor } from "@babel/traverse";
Expand Down Expand Up @@ -273,7 +269,7 @@ export default declare<PluginState>((api, options: Options) => {
}
}

const source = getDynamicImportSource(path.node);
const [source] = path.node.arguments;
const context = t.identifier(state.contextIdent);

path.replaceWith(
Expand All @@ -287,8 +283,10 @@ export default declare<PluginState>((api, options: Options) => {
[source],
)
: template.expression.ast`
new Promise(r => r(${source}))
.then(s => ${context}.import(s))
(source =>
new Promise(r => r("" + source))
.then(s => ${context}.import(s))
)(${source})
`,
);
}
Expand Down