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

Don't compile import() in development #12288

Merged
merged 8 commits into from Nov 18, 2020
Merged
Show file tree
Hide file tree
Changes from 5 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
49 changes: 2 additions & 47 deletions babel.config.js
Expand Up @@ -14,12 +14,10 @@ module.exports = function (api) {
const envOptsNoTargets = {
loose: true,
shippedProposals: true,
modules: false,
};
const envOpts = Object.assign({}, envOptsNoTargets);

const compileDynamicImport =
env === "test" || env === "development" || env === "test-legacy";

let convertESM = true;
let ignoreLib = true;
let includeRegeneratorRuntime = false;
Expand Down Expand Up @@ -111,9 +109,8 @@ module.exports = function (api) {
"@babel/proposal-object-rest-spread",
{ useBuiltIns: true, loose: true },
],
compileDynamicImport ? dynamicImportUrlToPath : null,
compileDynamicImport ? "@babel/plugin-proposal-dynamic-import" : null,

convertESM ? "@babel/proposal-export-namespace-from" : null,
convertESM ? "@babel/transform-modules-commonjs" : null,
].filter(Boolean),
overrides: [
Expand Down Expand Up @@ -159,45 +156,3 @@ module.exports = function (api) {

return config;
};

// !!! WARNING !!! Hacks are coming

// import() uses file:// URLs for absolute imports, while require() uses
// file paths.
// Since this isn't handled by @babel/plugin-transform-modules-commonjs,
// we must handle it here.
// However, fileURLToPath is only supported starting from Node.js 10.
// In older versions, we can remove the pathToFileURL call so that it keeps
// the original absolute path.
// NOTE: This plugin must run before @babel/plugin-transform-modules-commonjs,
// and assumes that the target is the current node version.
function dynamicImportUrlToPath({ template, env }) {
const currentNodeSupportsURL =
!!require("url").pathToFileURL && env() !== "test-legacy"; // test-legacy is run on legacy node versions without pathToFileURL support
if (currentNodeSupportsURL) {
return {
visitor: {
CallExpression(path) {
if (path.get("callee").isImport()) {
path.get("arguments.0").replaceWith(
template.expression.ast`
require("url").fileURLToPath(${path.node.arguments[0]})
`
);
}
},
},
};
} else {
// TODO: Remove in Babel 8 (it's not needed when using Node 10)
return {
visitor: {
CallExpression(path) {
if (path.get("callee").isIdentifier({ name: "pathToFileURL" })) {
path.replaceWith(path.get("arguments.0"));
}
},
},
};
}
}
3 changes: 1 addition & 2 deletions eslint/babel-eslint-parser/test/index.js
@@ -1,5 +1,4 @@
import path from "path";
import { pathToFileURL } from "url";
import escope from "eslint-scope";
import unpad from "dedent";
import { parseForESLint } from "../src";
Expand Down Expand Up @@ -80,7 +79,7 @@ describe("Babel and Espree", () => {
paths: [path.dirname(require.resolve("eslint"))],
});

espree = await import(pathToFileURL(espreePath));
espree = require(espreePath);
});

describe("compatibility", () => {
Expand Down
2 changes: 1 addition & 1 deletion package.json
Expand Up @@ -20,7 +20,7 @@
"@babel/eslint-parser": "workspace:*",
"@babel/eslint-plugin-development": "workspace:*",
"@babel/eslint-plugin-development-internal": "workspace:*",
"@babel/plugin-proposal-dynamic-import": "^7.10.4",
"@babel/plugin-proposal-export-namespace-from": "^7.12.1",
"@babel/plugin-proposal-object-rest-spread": "^7.11.0",
"@babel/plugin-transform-for-of": "^7.10.4",
"@babel/plugin-transform-modules-commonjs": "^7.10.4",
Expand Down
185 changes: 101 additions & 84 deletions packages/babel-core/test/config-chain.js
Expand Up @@ -6,6 +6,21 @@ import util from "util";
import escapeRegExp from "lodash/escapeRegExp";
import * as babel from "../lib";

// "minNodeVersion": "10.0.0" <-- For Ctrl+F when dropping node 10
const supportsESM = parseInt(process.versions.node) >= 12;

const isMJS = file => file.slice(-4) === ".mjs";
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: String#endsWith is available since Node.js 4

Suggested change
const isMJS = file => file.slice(-4) === ".mjs";
const isMJS = file => file.endsWith(".mjs");

Copy link
Contributor

Choose a reason for hiding this comment

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

can use https://nodejs.org/api/path.html#path_path_extname_path as well, dunno if it's any better, tho


const skipUnsupportedESM = (esm, name) => {
if (esm && !supportsESM) {
console.warn(
`Skipping ${name} because native ECMAScript modules are not supported.`,
);
return true;
}
return false;
};

// TODO: In Babel 8, we can directly uses fs.promises which is supported by
// node 8+
const pfs =
Expand Down Expand Up @@ -49,8 +64,13 @@ function loadOptions(opts) {
return babel.loadOptions({ cwd: __dirname, ...opts });
}

function loadOptionsAsync(opts) {
return babel.loadOptionsAsync({ cwd: __dirname, ...opts });
function loadOptionsAsync({ filename, cwd = __dirname }, mjs) {
if (mjs) {
// import() crashes with jest
return loadOptionsAsyncInSpawedProcess({ filename, cwd });
Copy link
Member Author

Choose a reason for hiding this comment

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

@SimenB This is needed to spawn tests using import() in a different process so that Jest doesn't have to handle them.

If you remove this line (and thus run the tests in the current Node.js process managed by Jest), and run node --experimental-vm-modules ./node_modules/.bin/jest babel-core/test/config-chain it crashes.

The file containing import() is https://github.com/babel/babel/blob/main/packages/babel-core/src/config/files/import.js, and you can Ctrl+F for "babel.config.mjs" in this file to find a test using import() internally.

I'll create a branch with a smaller test file to make it easier to isolate the problem (even if I'm not able to reproduce it in a fresh project containing only Jest).

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, this is weird. The importModuleDynamically callback isn't invoked at all - I'm guessing this is some edge case in Node's implementation

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it will be better if I post the reproduction to the Node.js repo then: even if the tests or Jest did something wrong (but from my superficial debugging I don't think so), Node.js shouldn't crash.

Copy link
Contributor

@SimenB SimenB Oct 30, 2020

Choose a reason for hiding this comment

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

From debugging - putting a breakpoint on the import() expression shows it is called with an URL with the following href: file:///var/folders/gj/0mygpdfn6598xh34njlyrqzc0000gn/T/babel-test-load-config-async-babel.config.mjsYKe5KD/babel.config.mjs. This file exists in disk and is valid ESM. Then doing "Step over next function call" crashes with a segfault without stopping the debugger.

PID 43445 received SIGSEGV for address: 0x0
0   segfault-handler.node               0x00000001055e3fa0 _ZL16segfault_handleriP9__siginfoPv + 304
1   libsystem_platform.dylib            0x00007fff6951f5fd _sigtramp + 29
2   ???                                 0x0000000000000007 0x0 + 7
3   node                                0x000000010033df4a _ZN2v88internal7Isolate38RunHostImportModuleDynamicallyCallbackENS0_6HandleINS0_6ScriptEEENS2_INS0_6ObjectEEE + 138
4   node                                0x00000001006f82f4 _ZN2v88internal25Runtime_DynamicImportCallEiPmPNS0_7IsolateE + 340
5   node                                0x0000000100a797b4 Builtins_CEntry_Return1_DontSaveFPRegs_ArgvInRegister_NoBuiltinExit + 52
[1]    43445 segmentation fault  node --experimental-vm-modules --inspect-brk ./node_modules/.bin/jest 

(via https://www.npmjs.com/package/segfault-handler)

Copy link
Member Author

Choose a reason for hiding this comment

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

}

return babel.loadOptionsAsync({ filename, cwd });
}

async function loadOptionsAsyncInSpawedProcess({ filename, cwd }) {
Expand All @@ -67,7 +87,10 @@ async function loadOptionsAsyncInSpawedProcess({ filename, cwd }) {
env: process.env,
},
);
if (stderr) {

const EXPERIMENTAL_WARNING = /\(node:\d+\) ExperimentalWarning: The ESM module loader is experimental\./;

if (stderr.replace(EXPERIMENTAL_WARNING, "").trim()) {
throw new Error(
"error is thrown in babel-load-options-async.mjs: stdout\n" +
stdout +
Expand All @@ -88,10 +111,11 @@ function pairs(items) {
return pairs;
}

async function getTemp(name, fixtureFolder = "config-files-templates") {
async function getTemp(name) {
const cwd = await pfs.mkdtemp(os.tmpdir() + path.sep + name);
const tmp = name => path.join(cwd, name);
const config = name => pfs.copyFile(fixture(fixtureFolder, name), tmp(name));
const config = name =>
pfs.copyFile(fixture("config-files-templates", name), tmp(name));
return { cwd, tmp, config };
}

Expand Down Expand Up @@ -1060,30 +1084,33 @@ describe("buildConfigChain", function () {
);
});

test.each(
[
"babel.config.json",
"babel.config.js",
"babel.config.cjs",
// We can't transpile import() while publishing, and it isn't supported
// by jest.
process.env.IS_PUBLISH ? "" : "babel.config.mjs",
].filter(Boolean),
)("should load %s asynchronously", async name => {
test.each([
"babel.config.json",
"babel.config.js",
"babel.config.cjs",
"babel.config.mjs",
JLHwung marked this conversation as resolved.
Show resolved Hide resolved
])("should load %s asynchronously", async name => {
const esm = isMJS(name);
if (skipUnsupportedESM(esm, `should load ${name} asynchronously`)) {
return;
}

const { cwd, tmp, config } = await getTemp(
`babel-test-load-config-async-${name}`,
);
const filename = tmp("src.js");

await config(name);

expect(await loadOptionsAsync({ filename, cwd })).toEqual({
...getDefaults(),
filename,
cwd,
root: cwd,
comments: true,
});
await expect(loadOptionsAsync({ filename, cwd }, esm)).resolves.toEqual(
{
...getDefaults(),
filename,
cwd,
root: cwd,
comments: true,
},
);
});

test.each(
Expand All @@ -1094,48 +1121,26 @@ describe("buildConfigChain", function () {
"babel.config.mjs",
]),
)("should throw if both %s and %s are used", async (name1, name2) => {
const { cwd, tmp, config } = await getTemp(
`babel-test-dup-config-${name1}-${name2}`,
);

// We can't transpile import() while publishing, and it isn't supported
// by jest.
const esm = isMJS(name1) || isMJS(name2);
if (
process.env.IS_PUBLISH &&
(name1 === "babel.config.mjs" || name2 === "babel.config.mjs")
skipUnsupportedESM(
esm,
`should throw if both ${name1} and ${name2} are used`,
)
) {
return;
}

const { cwd, tmp, config } = await getTemp(
`babel-test-dup-config-${name1}-${name2}`,
);

await Promise.all([config(name1), config(name2)]);

await expect(
loadOptionsAsync({ filename: tmp("src.js"), cwd }),
loadOptionsAsync({ filename: tmp("src.js"), cwd }, esm),
).rejects.toThrow(/Multiple configuration files found/);
});

if (process.env.IS_PUBLISH) {
test.each(["babel.config.mjs", ".babelrc.mjs"])(
"should load %s asynchronously",
async name => {
const { cwd, tmp, config } = await getTemp(
`babel-test-load-config-async-prepublish-${name}`,
"config-files-templates-prepublish",
);
const filename = tmp("src.js");
await config(name);
expect(
await loadOptionsAsyncInSpawedProcess({ filename, cwd }),
).toEqual({
...getDefaults(),
filename,
cwd,
root: cwd,
comments: true,
});
},
);
}
});

describe("relative", () => {
Expand Down Expand Up @@ -1181,25 +1186,30 @@ describe("buildConfigChain", function () {
".babelrc",
".babelrc.js",
".babelrc.cjs",
// We can't transpile import() while publishing, and it isn't supported
// by jest.
process.env.IS_PUBLISH ? "" : "babel.config.mjs",
".babelrc.mjs",
].filter(Boolean),
)("should load %s asynchronously", async name => {
const esm = isMJS(name);
if (skipUnsupportedESM(esm, `should load ${name} asynchronously`)) {
return;
}

const { cwd, tmp, config } = await getTemp(
`babel-test-load-config-${name}`,
);
const filename = tmp("src.js");

await config(name);

expect(await loadOptionsAsync({ filename, cwd })).toEqual({
...getDefaults(),
filename,
cwd,
root: cwd,
comments: true,
});
await expect(loadOptionsAsync({ filename, cwd }, esm)).resolves.toEqual(
{
...getDefaults(),
filename,
cwd,
root: cwd,
comments: true,
},
);
});

it("should load .babelignore", () => {
Expand All @@ -1220,23 +1230,24 @@ describe("buildConfigChain", function () {
".babelrc.json",
]),
)("should throw if both %s and %s are used", async (name1, name2) => {
const { cwd, tmp, config } = await getTemp(
`babel-test-dup-config-${name1}-${name2}`,
);

// We can't transpile import() while publishing, and it isn't supported
// by jest.
const esm = isMJS(name1) || isMJS(name2);
if (
process.env.IS_PUBLISH &&
(name1 === ".babelrc.mjs" || name2 === ".babelrc.mjs")
skipUnsupportedESM(
esm,
`should throw if both ${name1} and ${name2} are used`,
)
) {
return;
}

const { cwd, tmp, config } = await getTemp(
`babel-test-dup-config-${name1}-${name2}`,
);

await Promise.all([config(name1), config(name2)]);

await expect(
loadOptionsAsync({ filename: tmp("src.js"), cwd }),
loadOptionsAsync({ filename: tmp("src.js"), cwd }, esm),
).rejects.toThrow(/Multiple configuration files found/);
});

Expand All @@ -1260,17 +1271,23 @@ describe("buildConfigChain", function () {
${".babelrc.cjs"} | ${"babelrc-cjs-error"} | ${/Babelrc threw an error/}
${".babelrc.mjs"} | ${"babelrc-mjs-error"} | ${/Babelrc threw an error/}
${"package.json"} | ${"pkg-error"} | ${/Error while parsing JSON - /}
`("should show helpful errors for $config", async ({ dir, error }) => {
const filename = fixture("config-files", dir, "src.js");

// We can't transpile import() while publishing, and it isn't supported
// by jest.
if (process.env.IS_PUBLISH && dir === "babelrc-mjs-error") return;

await expect(
loadOptionsAsync({ filename, cwd: path.dirname(filename) }),
).rejects.toThrow(error);
});
`(
"should show helpful errors for $config",
async ({ config, dir, error }) => {
const esm = isMJS(config);
if (
skipUnsupportedESM(esm, `should show helpful errors for ${config}`)
) {
return;
}

const filename = fixture("config-files", dir, "src.js");

await expect(
loadOptionsAsync({ filename, cwd: path.dirname(filename) }, esm),
).rejects.toThrow(error);
},
);

it("loadPartialConfig should return a list of files that were extended", () => {
const filename = fixture("config-files", "babelrc-extended", "src.js");
Expand Down

This file was deleted.

This file was deleted.

@@ -1,8 +1,3 @@
// Until Jest supports native mjs, we must simulate it 🤷

module.exports = new Promise(resolve => resolve({
default: {
comments: true
}
}));
module.exports.__esModule = true;
export default {
comments: true,
};