Error when mixing ESM and CJS

joeldenning commented 3 years ago

In a project with "type": "module" in the package.json that also contains .cjs files, the .cjs files cannot access the global variables __filename, __dirname, etc that should be available to commonjs files.

I've created https://github.com/joeldenning/ncc-mixed-modules to demonstrate the problem.

~/c/ncc-mixed-modules (main|✔) $ node dist/index.js
file:///Users/joeldenning/code/ncc-mixed-modules/dist/index.js:6
console.log("dep.cjs executing", __filename)
                                 ^

ReferenceError: __filename is not defined in ES module scope
This file is being treated as an ES module because it has a '.js' file extension and '/Users/joeldenning/code/ncc-mixed-modules/dist/package.json' contains "type": "module". To treat it as a CommonJS script, rename it to use the '.cjs' file extension.
    at Object.135 (file:///Users/joeldenning/code/ncc-mixed-modules/dist/index.js:6:34)
    at __nccwpck_require__ (file:///Users/joeldenning/code/ncc-mixed-modules/dist/index.js:32:41)
    at file:///Users/joeldenning/code/ncc-mixed-modules/dist/index.js:52:66
    at file:///Users/joeldenning/code/ncc-mixed-modules/dist/index.js:56:3
    at ModuleJob.run (node:internal/modules/esm/module_job:175:25)
    at async Loader.import (node:internal/modules/esm/loader:178:24)
    at async Object.loadESM (node:internal/process/esm_loader:68:5)
    at async handleMainPromise (node:internal/modules/run_main:63:12)

guybedford commented 3 years ago

This would be a Webpack semantic in how webpack builds CommonJS.

One of the things to note is that __filename in a bundle is not well-defined because many files with many different __filename values bundled together into a final bundle will be in a bundle file that has another __filename value. So then the question is which one of those __filename values should be used. I don't think any bundlers have a good answer to this from a definition perspective yet. Browserify used to use /local/path.js as cwd-relative turning into absolute-url-relative paths, but I'm not sure if Webpack fully adopted that convention (//cc @sokra).

So a question to understand further would be - what is the use case you have in mind here that you're looking to handle? Then we can discuss ways to approach that use case rather.

joeldenning commented 3 years ago

In my case, I worked around it by replacing __filename with process.cwd(). The filename was there because it was part of a generated file, and it was used as part of `path.resolve(filename, 'some-other-file.js')`.

guybedford commented 3 years ago

Was the generated file an ES module or a CJS module?

joeldenning commented 3 years ago

The generated file was a CJS module - it was created by sequelize-cli as part of creating a database migration. For reference, here's the original file generated by sequelize-cli:

https://github.com/JustUtahCoders/flax-crm/blob/5c35bbe3f8818ea4f936fbef09fe85c0367b98ca/backend/DB/models/index.cjs

guybedford commented 3 years ago

So basically, we only rewrite __filename or __dirname when we are able to analyze what expression they are used in. If we can't do that analysis we leave them as-is and in ESM files they therefore remain undefined.

When building into CommonJS we do exactly the same thing, but __filename is then defined of course (just wrong effectively since the relative locations will break).

At the end of the day this is about use cases... I would be surprised if the use case was working with the naive CJS version anyway.

Again, regardless of outcome - this is a Webpack concern not an ncc concern. I've posted https://github.com/webpack/webpack/issues/14072 to track further.

appelgriebsch commented 2 years ago

Looks like it also mixes up import and require statements in the bundled mjs:

screenshot-2021-10-06-162911

As such its not possible to build with the current version. The version 0.28.x builds it just fine though (only cjs output).

algo7 commented 1 year ago

any news on this?

styfle commented 6 months ago

It looks like https://github.com/webpack/webpack/issues/14072 was finally merged and released in webpack@5.90.0.

The PR to upgrade webpack (https://github.com/vercel/ncc/pull/1158) is currently failing CI so if anyone wants to fix it, I can take a look, thanks!

RSS1102 commented 3 months ago

It looks like webpack/webpack#14072 was finally merged and released in webpack@5.90.0.

The PR to upgrade webpack (#1158) is currently failing CI so if anyone wants to fix it, I can take a look, thanks!

transfer to https://github.com/vercel/ncc/pull/1177

vercel / ncc

Error when mixing ESM and CJS #749