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.

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

Return rejected promise when stringify import specifier throws #15290

Merged
merged 6 commits into from Dec 23, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
33 changes: 33 additions & 0 deletions packages/babel-helper-module-transforms/src/dynamic-import.ts
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 All @@ -13,3 +14,35 @@ export function getDynamicImportSource(
? source
: (template.expression.ast`\`\${${source}}\`` as t.TemplateLiteral);
}

export function buildDynamicImport(
node: t.CallExpression,
runInThen: boolean,
builder: (specifier: t.Expression) => t.Expression,
): t.Expression {
const [specifier] = node.arguments;
if (
t.isStringLiteral(specifier) ||
(t.isTemplateLiteral(specifier) && specifier.quasis.length === 0)
) {
if (runInThen) {
return template.expression.ast`
Promise.resolve().then(() => ${builder(specifier)})
`;
} else return builder(specifier);
}

return template.expression.ast`
(specifier =>
new Promise(r => r(${
t.isTemplateLiteral(specifier)
? t.identifier("specifier")
: t.templateLiteral(
[t.templateElement({ raw: "" }), t.templateElement({ raw: "" })],
[t.identifier("specifier")],
)
}))
.then(s => ${builder(t.identifier("s"))})
)(${specifier})
`;
}
2 changes: 1 addition & 1 deletion packages/babel-helper-module-transforms/src/index.ts
Expand Up @@ -35,7 +35,7 @@ import type {
} from "./normalize-and-load-metadata";
import type { NodePath } from "@babel/traverse";

export { getDynamicImportSource } from "./dynamic-import";
export { buildDynamicImport, getDynamicImportSource } from "./dynamic-import";

export { default as getModuleName } from "./get-module-name";
export type { PluginOptions } from "./get-module-name";
Expand Down
@@ -1,5 +1,5 @@
define(["require"], function (_require) {
"use strict";

var modP = new Promise((_resolve, _reject) => _require(["mod"], imported => _resolve(babelHelpers.interopRequireWildcard(imported)), _reject));
var modP = Promise.resolve().then(() => new Promise((_resolve, _reject) => _require(["mod"], imported => _resolve(babelHelpers.interopRequireWildcard(imported)), _reject)));
});
@@ -1,3 +1,3 @@
define(["require"], function (_require) {
var modP = new Promise((_resolve, _reject) => _require(["mod"], imported => _resolve(imported), _reject));
var modP = Promise.resolve().then(() => new Promise((_resolve, _reject) => _require(["mod"], imported => _resolve(imported), _reject)));
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now the logic looks like this:

CommonJS (Deferred to then):

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):

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

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

});
@@ -1,3 +1,3 @@
define(["require"], function (_require) {
var modP = new Promise((_resolve, _reject) => _require(["mod"], imported => _resolve(babelHelpers.interopRequireWildcard(imported)), _reject));
var modP = Promise.resolve().then(() => new Promise((_resolve, _reject) => _require(["mod"], imported => _resolve(babelHelpers.interopRequireWildcard(imported)), _reject)));
});
@@ -0,0 +1 @@
import(2);
@@ -0,0 +1,3 @@
define(["require"], function (_require) {
(specifier => new Promise(r => r(`${specifier}`)).then(s => new Promise((_resolve, _reject) => _require([s], imported => _resolve(babelHelpers.interopRequireWildcard(imported)), _reject))))(2);
});
Expand Up @@ -8,5 +8,5 @@ define(["require", "exports", "foo"], function (_require, _exports, _foo) {
_foo = babelHelpers.interopRequireDefault(_foo);
var mod;
_exports.mod = mod;
new Promise((_resolve, _reject) => _require(["mod"], imported => _resolve(babelHelpers.interopRequireWildcard(imported)), _reject)).then(m => _exports.mod = mod = m);
Promise.resolve().then(() => new Promise((_resolve, _reject) => _require(["mod"], imported => _resolve(babelHelpers.interopRequireWildcard(imported)), _reject))).then(m => _exports.mod = mod = m);
});
@@ -0,0 +1 @@
module.exports = 1;
@@ -0,0 +1,3 @@
let x = 1;
return expect(import(`./${x}.js`))
.resolves.toHaveProperty("default", 1);
@@ -0,0 +1,5 @@
{
"parserOpts": {
"allowReturnOutsideFunction": true
}
}
@@ -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");
@@ -0,0 +1,6 @@
return expect(import({
[Symbol.toPrimitive](hint) {
if (hint === "string") return "./foo.js";
return null;
}
})).resolves.toHaveProperty("default", "foo");
@@ -0,0 +1 @@
module.exports = "foo";
@@ -0,0 +1,5 @@
{
"parserOpts": {
"allowReturnOutsideFunction": true
}
}
@@ -0,0 +1 @@
module.exports = 1;
@@ -0,0 +1,7 @@
let i = 0;
const promise = expect(import({ toString: () => `./${++i}.js` }))
.resolves.toHaveProperty("default", 1);
expect(i).toBe(1);
++i;
expect(i).toBe(2);
return promise;
@@ -0,0 +1,5 @@
{
"parserOpts": {
"allowReturnOutsideFunction": true
}
}
@@ -0,0 +1,2 @@
return expect(import({ toString: () => { throw "toString failed"; } }))
.rejects.toBe("toString failed");
@@ -0,0 +1,5 @@
{
"parserOpts": {
"allowReturnOutsideFunction": true
}
}
@@ -0,0 +1,2 @@
const x = 1;
import(`./${x}.js`);
@@ -0,0 +1,2 @@
const x = 1;
(specifier => new Promise(r => r(specifier)).then(s => babelHelpers.interopRequireWildcard(require(s))))(`./${x}.js`);
@@ -1 +1 @@
Promise.resolve(`${2}`).then(s => babelHelpers.interopRequireWildcard(require(s)));
(specifier => new Promise(r => r(`${specifier}`)).then(s => babelHelpers.interopRequireWildcard(require(s))))(2);
@@ -0,0 +1 @@
import(2);
@@ -0,0 +1,10 @@
System.register([], function (_export, _context) {
"use strict";

return {
setters: [],
execute: function () {
(specifier => new Promise(r => r(`${specifier}`)).then(s => _context.import(s)))(2);
}
};
});
21 changes: 13 additions & 8 deletions packages/babel-plugin-transform-modules-amd/src/index.ts
@@ -1,5 +1,6 @@
import { declare } from "@babel/helper-plugin-utils";
import {
buildDynamicImport,
isModule,
rewriteModuleStatementsAndPrepareHeader,
type RewriteModuleStatementsAndPrepareHeaderOptions,
Expand All @@ -9,7 +10,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 @@ -99,14 +99,19 @@ export default declare<State>((api, options: Options) => {
if (!noInterop) result = wrapInterop(path, result, "namespace");

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

Expand Down
Expand Up @@ -3,7 +3,7 @@

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

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

const source = getDynamicImportSource(path.node);

const replacement =
t.isStringLiteral(source) ||
(t.isTemplateLiteral(source) && source.quasis.length === 0)
? template.expression.ast`
Promise.resolve().then(() => ${buildRequire(source, file)})
`
: template.expression.ast`
Promise.resolve(${source}).then(
s => ${buildRequire(t.identifier("s"), file)}
)
`;

path.replaceWith(replacement);
path.replaceWith(
buildDynamicImport(path.node, true, specifier =>
buildRequire(specifier, file),
),
);
}
16 changes: 9 additions & 7 deletions packages/babel-plugin-transform-modules-systemjs/src/index.ts
Expand Up @@ -2,9 +2,9 @@ import { declare } from "@babel/helper-plugin-utils";
import hoistVariables from "@babel/helper-hoist-variables";
import { template, types as t } from "@babel/core";
import {
rewriteThis,
buildDynamicImport,
getModuleName,
getDynamicImportSource,
rewriteThis,
} from "@babel/helper-module-transforms";
import type { PluginOptions } from "@babel/helper-module-transforms";
import { isIdentifierName } from "@babel/helper-validator-identifier";
Expand Down Expand Up @@ -274,12 +274,14 @@ export default declare<PluginState>((api, options: Options) => {
}

path.replaceWith(
t.callExpression(
t.memberExpression(
t.identifier(state.contextIdent),
t.identifier("import"),
buildDynamicImport(path.node, false, specifier =>
t.callExpression(
t.memberExpression(
t.identifier(state.contextIdent),
t.identifier("import"),
),
[specifier],
),
[getDynamicImportSource(path.node)],
),
);
}
Expand Down
@@ -1,7 +1,9 @@
define(["require"], function (_require) {
new Promise(function (_resolve, _reject) {
return _require(["foo"], function (imported) {
return _resolve(babelHelpers.interopRequireWildcard(imported));
}, _reject);
Promise.resolve().then(function () {
return new Promise(function (_resolve, _reject) {
return _require(["foo"], function (imported) {
return _resolve(babelHelpers.interopRequireWildcard(imported));
}, _reject);
});
});
});
@@ -1,3 +1,3 @@
define(["require"], function (_require) {
new Promise((_resolve, _reject) => _require(["foo"], imported => _resolve(babelHelpers.interopRequireWildcard(imported)), _reject));
Promise.resolve().then(() => new Promise((_resolve, _reject) => _require(["foo"], imported => _resolve(babelHelpers.interopRequireWildcard(imported)), _reject)));
});