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 2 commits
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/module/output.js
View file
Open in desktop
@@ -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)));
});
2 changes: 1 addition & 1 deletion packages/babel-plugin-proposal-dynamic-import/test/fixtures/amd/no-interop/output.js
View file
Open in desktop
@@ -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.

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)}
)
)
`,
),

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

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

});
2 changes: 1 addition & 1 deletion packages/babel-plugin-proposal-dynamic-import/test/fixtures/amd/script/output.js
View file
Open in desktop
@@ -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)));
});
1 change: 1 addition & 0 deletions packages/babel-plugin-proposal-dynamic-import/test/fixtures/amd/to-string/input.js
View file
Open in desktop
@@ -0,0 +1 @@
import(2);
3 changes: 3 additions & 0 deletions packages/babel-plugin-proposal-dynamic-import/test/fixtures/amd/to-string/output.js
View file
Open in desktop
@@ -0,0 +1,3 @@
define(["require"], function (_require) {
(source => new Promise(r => r("" + source)).then(s => new Promise((_resolve, _reject) => _require([s], imported => _resolve(babelHelpers.interopRequireWildcard(imported)), _reject))))(2);
});
2 changes: 1 addition & 1 deletion ...babel-plugin-proposal-dynamic-import/test/fixtures/amd/with-other-import-export/output.js
View file
Open in desktop
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);
});
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");
1 change: 1 addition & 0 deletions ...ges/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-string-order/1.js
View file
Open in desktop
@@ -0,0 +1 @@
module.exports = 1;
7 changes: 7 additions & 0 deletions .../babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-string-order/exec.js
View file
Open in desktop
@@ -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;
5 changes: 5 additions & 0 deletions ...l-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-string-order/options.json
View file
Open in desktop
@@ -0,0 +1,5 @@
{
"parserOpts": {
"allowReturnOutsideFunction": true
}
}
2 changes: 2 additions & 0 deletions .../babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-string-throw/exec.js
View file
Open in desktop
@@ -0,0 +1,2 @@
return expect(import({ toString: () => { throw "toString failed"; } }))
.rejects.toBe("toString failed");
5 changes: 5 additions & 0 deletions ...l-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-string-throw/options.json
View file
Open in desktop
@@ -0,0 +1,5 @@
{
"parserOpts": {
"allowReturnOutsideFunction": true
}
}
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 @@
Promise.resolve(`${2}`).then(s => babelHelpers.interopRequireWildcard(require(s)));
(source => new Promise(r => r("" + source)).then(s => babelHelpers.interopRequireWildcard(require(s))))(2);
1 change: 1 addition & 0 deletions packages/babel-plugin-proposal-dynamic-import/test/fixtures/systemjs/to-string/input.js
View file
Open in desktop
@@ -0,0 +1 @@
import(2);
10 changes: 10 additions & 0 deletions packages/babel-plugin-proposal-dynamic-import/test/fixtures/systemjs/to-string/output.js
View file
Open in desktop
@@ -0,0 +1,10 @@
System.register([], function (_export, _context) {
"use strict";

return {
setters: [],
execute: function () {
(source => new Promise(r => r("" + source)).then(s => _context.import(s)))(2);
}
};
});
35 changes: 26 additions & 9 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,15 +97,33 @@ export default declare<State>((api, options: Options) => {
let result: t.Node = t.identifier("imported");
if (!noInterop) result = wrapInterop(path, result, "namespace");

const [source] = path.node.arguments;

path.replaceWith(
template.expression.ast`
new Promise((${resolveId}, ${rejectId}) =>
${requireId}(
[${getDynamicImportSource(path.node)}],
imported => ${t.cloneNode(resolveId)}(${result}),
${t.cloneNode(rejectId)}
)
)`,
t.isStringLiteral(source) ||
(t.isTemplateLiteral(source) && source.quasis.length === 0)
? template.expression.ast`
Promise.resolve()
.then(() => new Promise((${resolveId}, ${rejectId}) =>
${requireId}(
[${source}],
imported => ${t.cloneNode(resolveId)}(${result}),
${t.cloneNode(rejectId)}
)
))
`
: template.expression.ast`
(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
10 changes: 5 additions & 5 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,9 +28,10 @@ export function transformDynamicImport(
Promise.resolve().then(() => ${buildRequire(source, file)})
`
: template.expression.ast`
Promise.resolve(${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
31 changes: 19 additions & 12 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,14 +269,25 @@ export default declare<PluginState>((api, options: Options) => {
}
}

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

path.replaceWith(
t.callExpression(
t.memberExpression(
t.identifier(state.contextIdent),
t.identifier("import"),
),
[getDynamicImportSource(path.node)],
),
t.isStringLiteral(source) ||
(t.isTemplateLiteral(source) && source.quasis.length === 0)
? t.callExpression(
t.memberExpression(
t.identifier(state.contextIdent),
t.identifier("import"),
),
[source],
)
: template.expression.ast`
(source =>
new Promise(r => r("" + source))
.then(s => ${context}.import(s))
)(${source})
`,
);
}
},
Expand Down
10 changes: 6 additions & 4 deletions packages/babel-preset-env/test/fixtures/dynamic-import-babel-7/modules-amd/output.js
View file
Open in desktop
@@ -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);
});
});
});
2 changes: 1 addition & 1 deletion packages/babel-preset-env/test/fixtures/dynamic-import/modules-amd/output.js
View file
Open in desktop
@@ -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)));
});