[commonjs] - should not compile require

[Olyno]

• Rollup Plugin Name: commonjs
• Rollup Plugin Version: 11.1.0
• Rollup Version: 2.7.3
• Operating System (or Browser): Windows 10 or ElectronJS (chromium)
• Node Version: 14.0.0

How Do We Reproduce?

1. Clone https://github.com/Olyno/repro-rollup-issue
2. Install dependencies (npm i)
3. Run App (npm run dev)

Expected Behavior

Should not compile require syntax, but only es6 syntaxes (e.g import, export), or simply when we precise it to rollup. It should works fine with nodeIntegration of ElectronJS. It worked fine before I did a upgrade latest command, and now it doesn't work anymore.

Actual Behavior

In the console:

rollup v2.7.3
bundles src/main.js → public\build\bundle.js...
(!) Missing shims for Node.js built-ins
Creating a browser bundle that depends on 'path'. You might need to include https://www.npmjs.com/package/rollup-plugin-node-builtins
LiveReload enabled
(!) Unresolved dependencies
https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency
fs (imported by src\store.js, fs?commonjs-external)
path (imported by src\store.js, path?commonjs-external)
(!) Missing global variable names
Use output.globals to specify browser global variable names corresponding to external modules
fs (guessing 'fs')
path (guessing 'path')
created public\build\bundle.js in 321ms

[lukastaegert]

Your issue was caused by #206 where behaviour was changed to also process "Webpack style". mixed modules. To me, your use case looks important and I would assume to add a switch, e.g. processMixedModules: false or similar to switch back to the old behaviour that ignores any module containing ESM import or export statements would fix it. Would do you think @danielgindi ?

[danielgindi]

Your issue was caused by #206 where behaviour was changed to also process "Webpack style". mixed modules. To me, your use case looks important and I would assume to add a switch, e.g. processMixedModules: false or similar to switch back to the old behaviour that ignores any module containing ESM import or export statements would fix it. Would do you think @danielgindi ?

A kill switch is also acceptable. I think we can be smarter and actually choose our behavior on the fly, but a kill switch will allow more fine-grained control. I would like to actually have proper tests in place for these use cases which rollup probably never considered before...

[lukastaegert]

If there are some ideas how we can make Electron support even smarter, that would be great because I think there are more and more people who have this use-case. So to my understanding in an Electron application you have like two kind of processes, a "main process" and one or more "render processes". A render process is just a regular Web page and would very much profit from being bundled as ESM. However, Electron injects a global variable require that allows getting access to Node libraries in the browser, see https://www.electronjs.org/docs/tutorial/application-architecture

Not sure if the browser could even require relative files, but a rule of thumb is probably that require should not be touched at all but left unprocessed in such a scenario. If a kill switch is added, the documentation might actually mention its usefulness for Electron applications.

[danielgindi]

@lukastaegert To avoid require from being touched at all- they could just omit the commonjs module... or am I missing something?

[lukastaegert]

True, the problem is that the frontend code can still have CJS dependencies. So of course what you can do is fine-tune your include/exclude options to only target e.g. node_modules, but looking at the other issue I think there would be more use for an option to ignore mixed modules.

[stale]

Hey folks. This issue hasn't received any traction for 60 days, so we're going to close this for housekeeping. If this is still an ongoing issue, please do consider contributing a Pull Request to resolve it. Further discussion is always welcome even with the issue closed. If anything actionable is posted in the comments, we'll consider reopening it.# Comment to post when closing a stale issue. ⓘ