Skip transpilation of CommonJS modules #8
Conversation
| // 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 }; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
| // 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 }; | |
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Set babel preset env's
modulesoption tofalse. 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.
| // NOTE: transform-modules-commonjs doesn't work properly. | ||
| // We put a branch here just for consistency. | ||
| format: transformed.sourceType === "module" ? "module" : "commonjs", |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I don't know why
transformed.sourceTypewould 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.
| it(`allows loading CJS modules`, async () => { | ||
| const example = await import("./fixtures/basic/cjs.cjs"); | ||
| assert.deepEqual(example.default, { | ||
| cjs: "cjs", | ||
| }); | ||
| }); |
There was a problem hiding this comment.
See #7 (comment) - I don't think the bug reported is a problem with node-loader-babel, since relying on ESM importing named exports from CJS modules is not reliable or guaranteed and is a fairly experimental and new part of NodeJS that was implemented by Guy Bedford's lexer.
The choice of using NodeJS' normal loader for commonjs files versus using babel is something that should be provided as an option to the user of node-loader-babel. Some users will want babel to compile and others won't.
So I think the best way to solve this would be to provide a way for the user to customize which files are compiled by babel or not.
| // 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 }; | ||
| } |
There was a problem hiding this comment.
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?
| // 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 }; | |
| } |
| // NOTE: transform-modules-commonjs doesn't work properly. | ||
| // We put a branch here just for consistency. | ||
| format: transformed.sourceType === "module" ? "module" : "commonjs", |
There was a problem hiding this comment.
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?
| // 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. |
There was a problem hiding this comment.
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.
|
Closing this in favor of #10. After diagnosing the issue locally, I had already written a test similar to the one in this PR and my fix had a few small differences in implementation with this PR so I decided to just create a new one. Thanks @qnighy for your work here - your solution was a good one. And thanks @make-github-pseudonymous-again for your research about the problem, which helped me realize that NodeJS itself returns |
|
@joeldenning Thank you Joel! |
Currently, it transpiles CommonJS modules in an unexpected way.
Instead of supporting them, this PR entirely skips transpilation of CommonJS modules. Rationale:
*.cjsinputs before it reaches the loader. Therefore it cannot provide full transpilation ability; only the syntaxes Node.js knows can be transpiled.@babel/nodeor@babel/registercan handle them instead.Fixes #7