Skip to content

Skip transpilation of CommonJS modules #8

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

Closed
qnighy wants to merge 1 commit into from
Closed

Skip transpilation of CommonJS modules #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions lib/node-loader-babel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,23 @@ function isBabelConfigFile(filename) {

export async function load(url, context, defaultLoad) {
if (useLoader(url)) {
const { source } = await defaultLoad(url, context, defaultLoad);
const { source, format } = await defaultLoad(url, context, defaultLoad);

// Skip transpilation of CommonJS modules.
// These modules are already preprocessed by Node.js,
// so we cannot parse the non-standard syntaxes like JSX and TypeScript.
// Their transpilation is better handled separately by @babel/register or @babel/node.
if (format !== "module") {
return { source, format };
}
Comment on lines +24 to +30
Copy link

@make-github-pseudonymous-again make-github-pseudonymous-again Dec 22, 2021

Choose a reason for hiding this comment

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

I am nitpicking here but other formats include builtin, json, and wasm (see https://nodejs.org/api/esm.html#loadurl-context-defaultload). The logic makes sense. The comment could be improved.

Copy link

@internettrans internettrans Dec 27, 2021

Choose a reason for hiding this comment

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

I don't think that skipping compilation of commonjs modules is a good universal assumption, since babel is completely capable of compiling commonjs modules. I agree skipping json, wasm, and builtin formats makes sense. As a separate feature, I think that we could let users of node-loader-babel customize which files are compiled by providing their own useLoader function. But that wouldn't be part of this PR.

As far as I know, Babel doesn't change the format of modules by default (unless you add a plugin that does it). Babel preset env doesn't seem to do so by default (see this repl). So I would think it's safe to let babel just compile the file like normal?

Suggested change
// Skip transpilation of CommonJS modules.
// These modules are already preprocessed by Node.js,
// so we cannot parse the non-standard syntaxes like JSX and TypeScript.
// Their transpilation is better handled separately by @babel/register or @babel/node.
if (format !== "module") {
return { source, format };
}
if (format !== "module" && format !== "commonjs") {
return { source, format };
}

Copy link

@make-github-pseudonymous-again make-github-pseudonymous-again Dec 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think that skipping compilation of commonjs modules is a good universal assumption, since babel is completely capable of compiling commonjs modules. I agree skipping json, wasm, and builtin formats makes sense. As a separate feature, I think that we could let users of node-loader-babel customize which files are compiled by providing their own useLoader function. But that wouldn't be part of this PR.

Looking forward what you say makes sense. It may be that one day node loader handles the source key for {format: 'commonjs'}. But insisting on commonjs being transpiled now makes little sense. As far as I understand, with the code of this PR, a commonjs input will have const {format, source} = await defaultLoad(...); such that format = 'commonjs' and source = null. Babel gracefully handles null by producing null with little overhead. If one day source is non-null and has meaning for format = 'commonjs' suddenly this loader will start transpiling CJS input without warning. Not sure if this is a real stability threat but I feel like it is a bad idea to leave this open for this code to be general just in the case of "if ...".

I agree this behavior could be configurable by the user of node-loader-babel. I just think the default should be to only process 'module' input.

Note that without this PR, when input format is commonjs, input and output source are null and output format is module which is the reason of the bug described in #7.

Copy link

@make-github-pseudonymous-again make-github-pseudonymous-again Dec 29, 2021

Choose a reason for hiding this comment

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

As far as I know, Babel doesn't change the format of modules by default (unless you add a plugin that does it). Babel preset env doesn't seem to do so by default (see this repl). So I would think it's safe to let babel just compile the file like normal?

In this REPL, enabling @babel/preset-env does transform ESM input into CJS output. How do you configure it so that it does not change format?

Copy link

@internettrans internettrans Jan 4, 2022

Choose a reason for hiding this comment

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

In this REPL, enabling @babel/preset-env does transform ESM input into CJS output. How do you configure it so that it does not change format?

Set babel preset env's modules option to false. The "auto" value also should work if we have called babel with the proper options

https://babeljs.io/docs/en/babel-preset-env#modules

I just think the default should be to only process 'module' input.

I can see how that would help solve the immediate problem, but don't think it's actually the proper behavior for node-loader-babel as a whole. If we can get babel to output the correct module format, there's no reason to avoid compilation of CJS files.

with the code of this PR, a commonjs input will have const {format, source} = await defaultLoad(...); such that format = 'commonjs' and source = null

Why would source be null? Does nodejs' default loader not provide commonjs source as a string? If so, then perhaps your suggestion of disabling would make more sense. But I thought that nodejs would give us the source as a string.

Copy link

@make-github-pseudonymous-again make-github-pseudonymous-again Jan 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Set babel preset env's modules option to false. The "auto" value also should work if we have called babel with the proper options

OK. I cannot find how to do this in the REPL, maybe it is not possible.


const filename = urlModule.fileURLToPath(url);
// Babel config files can themselves be ES modules,
// but we cannot transform those since doing so would cause an infinite loop.
if (isBabelConfigFile(filename)) {
return {
source,
format: /\.(c|m)?js$/.test(filename) ? "module" : "json",
format,
};
}

Expand All @@ -43,9 +51,9 @@ export async function load(url, context, defaultLoad) {

return {
source: transformed.code,
// Maybe a shaky assumption
// TODO: look at babel config to see whether it will output ESM/CJS or other formats
format: "module",
// NOTE: transform-modules-commonjs doesn't work properly.
Copy link

@internettrans internettrans Dec 27, 2021

Choose a reason for hiding this comment

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

Which bug in transform-modules-commonjs you're referring to?

I don't think we ever need to forcibly change the module format - see my comment above about how I don't think babel will change module format unless explicitly told to do so via plugin, which is something controlled via babel config.

// We put a branch here just for consistency.
format: transformed.sourceType === "module" ? "module" : "commonjs",
Comment on lines +54 to +56
Copy link

@make-github-pseudonymous-again make-github-pseudonymous-again Dec 22, 2021

Choose a reason for hiding this comment

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

Does transformed.sourceType really reflect the output format? Perhaps it makes more sense to assume output is module or fail if it is not given source is not applicable for format: "commonjs" (see https://nodejs.org/api/esm.html#loadurl-context-defaultload).

Copy link

@internettrans internettrans Dec 27, 2021

Choose a reason for hiding this comment

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

I don't know why transformed.sourceType would not reflect the real output format. Are there bugs in babel or common plugins that make this wrong?

Copy link

@make-github-pseudonymous-again make-github-pseudonymous-again Dec 28, 2021

Choose a reason for hiding this comment

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

I don't know why transformed.sourceType would not reflect the real output format. Are there bugs in babel or common plugins that make this wrong?

I do not know, I am just confused as to whether sourceType corresponds to the input sourceType or the output sourceType, since source carries an input meaning to me. Experimental node loader uses the source keyword for both "input" and "output" but the babel equivalent is transformed.code. I could not find the Babel documentation that explains what transformed.sourceType means. If you can find it please share.

};
} else {
return defaultLoad(url, context, defaultLoad);
Expand Down
7 changes: 7 additions & 0 deletions test/basic.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,13 @@ describe(`basic babel usage`, () => {
});
});

it(`allows loading CJS modules`, async () => {
const example = await import("./fixtures/basic/cjs.cjs");
assert.deepEqual(example.default, {
cjs: "cjs",
});
});
Comment on lines +13 to +18
Copy link

@make-github-pseudonymous-again make-github-pseudonymous-again Dec 22, 2021

Choose a reason for hiding this comment

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

This is neat!


it(`supports ES module babel config files`, async () => {
const mjsConfig = await import("./fixtures/mjs-config/main.js");
assert.deepEqual(mjsConfig.default, {
Expand Down
1 change: 1 addition & 0 deletions test/fixtures/basic/cjs.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
exports.cjs = "cjs";