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

CJS support #7

Closed
make-github-pseudonymous-again opened this issue Dec 10, 2021 · 7 comments
Closed

CJS support #7

make-github-pseudonymous-again opened this issue Dec 10, 2021 · 7 comments

Comments

@make-github-pseudonymous-again

I found at least one offensive line here:

// Maybe a shaky assumption
// TODO: look at babel config to see whether it will output ESM/CJS or other formats
format: "module",

If I replace the format definition with format: /\.cjs$/.test(filename) ? "commonjs" : "module", it works for me so I assume something was overlooked here. This may not be the best way to handle this but at least it shows I can fix it to work for my use case.

If I do not do that, I get tons of errors similar to SyntaxError: The requested module '#foo' does not provide an export named bar when importing some commonjs module. I am also using @node-loader/import-maps so maybe there is some interdependence there?

@make-github-pseudonymous-again
Copy link
Author

I found a better way is to rely on the original format through defaultLoad, for exampe:

const {format, source} = await defaultLoad(url, context, defaultLoad);

This solves my use-case but is still not a solution to getting the output format through babel. Although I am not sure it makes sense for babel to transform from ESM to CJS in this particular case because the load hook cannot emit source for the commonjs format (see https://nodejs.org/api/esm.html#loadurl-context-defaultload).

@joeldenning
Copy link
Contributor

If I do not do that, I get tons of errors similar to SyntaxError: The requested module '#foo' does not provide an export named bar when importing some commonjs module. I am also using @node-loader/import-maps so maybe there is some interdependence there?

This is expected, since commonjs modules do not provide named exports. Recently, Guy Bedford added a lexer to node that attempts to provide named exports for commonjs modules, but this is not guaranteed to work in all cases. See https://nodejs.org/dist/latest-v17.x/docs/api/esm.html#import-statements, which says "When importing CommonJS modules, the module.exports object is provided as the default export. Named exports may be available, provided by static analysis as a convenience for better ecosystem compatibility."

Is the problem you're encountering that the static analysis of that lexer is skipped when a loader is applied? If so, I think that the best way to solve that would be to allow the user of node-loader-babel to customize the useLoader function.

@make-github-pseudonymous-again
Copy link
Author

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

OK. I thought I had pinpointed it. See explanation in #8 (comment). I will double-check and come back to you.

PS: BTW @joeldenning, thanks for your great work on this software. Other than this minor "issue", it has worked wonders so far.

@make-github-pseudonymous-again
Copy link
Author

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

I created a reproduction repository at https:/bisectifiate/node-loader-babel-commonjs.

I tested the following (:heavy_check_mark: means it works, :heavy_multiplication_x: means it does not work):

  • Importing named commonjs exports from module:
    • without loader ✔️
    • with @node-loader/babel ✖️
    • with a custom babel loader ✔️
  • Importing named module exports from module:
    • without loader ✔️
    • with @node-loader/babel ✔️
    • with a custom babel loader ✔️
  • Importing default module exports from module:
    • without loader ✔️
    • with @node-loader/babel ✔️
    • with a custom babel loader ✔️

It seems commonjs named exports work without a loader or with a patched babel loader.

To rule out static analysis as the culprit, there is this PR which imports the commonjs source with a default import (import x from) rather than a named import (import {y} from). @node-loader/babel still fails in this case even though we do not use named exports requiring static analysis. The error is:

The requested module '../../src/commonjs-named.cjs' does not provide an export named 'default'

Note that this use case works without a loader or with a patched babel loader.

What I think is happening with @node-loader/babel is that a commonjs source is passed to babel as null, babel outputs null, and @node-loader/babel wraps this null as {format: 'module', source: null} which creates an empty module, with no named or default exports. If that is indeed the case, what @node-loader/babel should be doing, and that is what the patched version and #8 do, is to output {format: 'commonjs', source: null} when given an input with format commonjs (and hence with source null). See the table at https://nodejs.org/api/esm.html#loadurl-context-defaultload that says source is not applicable when format is 'commonjs':

The value of source is ignored for type 'commonjs' because the CommonJS module loader does not provide a mechanism for the ES module loader to override the CommonJS module return value. This limitation might be overcome in the future.

Just for your information, importing the default export from a commonjs input (exports = foobar) does not work at all (it does not work in any of the configurations: loader or no loader). See this PR. At the moment, I am not interested in making this kind of default export work. EDIT: OK, it does work if you use module.exports = foobar.

@make-github-pseudonymous-again
Copy link
Author

What I think is happening with @node-loader/babel is that a commonjs source is passed to babel as null, babel outputs null, and @node-loader/babel wraps this null as {format: 'module', source: null} which creates an empty module, with no named or default exports.

I was able to replicate @node-loader/babel's behavior with a minimal amount of code here. What happens is that babel transpiles null to the empty string ''. Transpiling to null would yield a more direct error. The conclusion stays the same however.

@joeldenning
Copy link
Contributor

Fix released in https:/node-loader/node-loader-babel/releases/tag/v2.0.1

@joeldenning
Copy link
Contributor

Added a bit of documentation that's available at https:/node-loader/node-loader-babel#commonjs-modules

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants