-
-
Notifications
You must be signed in to change notification settings - Fork 593
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
[commonjs] Discussion and overview of (nearly) all current interop issues + ideas how to move forward #481
Comments
I'm blown away by how thorough this is. Thank you for taking the time to outline! I don't have much to add here, other than to just add some color / advocate for "Importing ESM from CJS" as something that should be considered supported (above, you mention "if we support this"). This is common in applications and anything bundling 3rd part code, where an ESM package depends on a CJS package. Its been a problem in Snowpack before that most users coming from Webpack don't expect. |
Thanks, I changed the wording. I also explicitly numbered all improvement suggestions now and added a summary section that contains an "action plan" and some suggested order in which those could be tackled. I will gladly do the improvements in Rollup core but would be more than happy for support in improving the plugin. Would be especially nice to have @danielgindi on board here, but other contributors are heartily welcome as well. |
Just had a very brief read, Lukas this is great work and looks like it will be a great alignment. //cc @sokra for visibility. Both of my main points I've mentioned previously but will state them again. The first concern I still have here is that Node.js does not implement the So RollupJS working differently for this interop may cause breaks between Node.js semantics and RollupJS semantics. In addition the edge case for Object.defineProperty(exports, '__esModule', true)
exports.default = 'asdf' where Node.js does have a PR for named exports that would add __esModule interop just for the esmodule default override case above, the current consensus was to release this behind a flag initially for testing soon, then if there is greater user interest it could be unflagged. Basing that unflagging decision on user feedback and real package support is important though as it uses a heuristical analysis approach. The second point is more of a suggestion I've mentioned before that it could be useful to have 'auto' only work for the entry points, since that is a common use case for this feature. Perhaps 'autoentry' or something. This is additive work certainly but may smooth the transition. |
This is true and definitely a concern. Adding a "strictNodeSemantics" mode that does not implement this interop or allow named exports would be a prudent step forward, especially if it also bases ESM/CJS detection solely on Node semantics. This would be a larger undertaking, however, as it should ideally be based on ' @rollup/plugin-node-resolve` providing access to package files, so I would put it down for "lager enhancements". But as I am sure you are aware, this interop pattern is implemented by most other major tools in the ecosystem (I am aware Babel, TypeScript, Webpack) so I think we should support it out of the box. Nice to hear there are considerations to port it to Node as this will finally bring alignment across the whole ecosystem.
Are you referring to Rollup's Or are you referring to my suggestion for |
The PR for named exports support would not bring __esModule interop to any case other than the case where a CommonJS module exports both a default export and a __esModule export. But when named exports are analyzed and gathered onto the module namespace like Yes interop is an art not a science though. That said, users will likely simply shy away from the messier parts naturally.
Ah yes it sounds like I'm confusing that with your |
Taking my hat off. This is an intensively thorough work! I'm putting this on my desktop, to re-read and start to discuss here in the following days. Having a pretty busy weekend so will probably start on Sunday. |
@guybedford Even with this change the following would happen: // module.cjs
exports.__esModule = true;
exports.default = 42; import x from "./module.cjs";
console.log(x);
// Node.js: { default: 42 }
// Everybody else: 42 I get why you are doing that (To not introduce an breaking change), but if I would be in your place I would argue that this is a bug and should be fixed even while it's a breaking change. ESM in Node.js is still experimental so nobody can complain about that... |
I created a draft PR for extending syntheticNamedExports to support an arbitrary export as base in rollup/rollup#3657. Core functionality is working but some edge cases will still be refined. Update: The PR is now ready for review and includes the necessary documentation. |
No problems. I will start working on the Rollup core side of things for now anyway, and will not merge PRs too soon. |
I also created a PR for the new warning in rollup/rollup#3659 |
I'm very interested in some the cases which prevent deoptimization. As you know I'm reusing rollup plugins in es-dev-server (and in a new generation of tools at https://github.com/modernweb-dev/web). I'm only using the single file transform API, so I'm only able to rely on the generated named exports. Right now, any occurrence of Do we have a maintainer who is comfortable with the commonjs plugin to implement these changes? It's a pretty daunting piece of code, but I could try and dig into it. |
Besides myself, @danielgindi is the one who did most work on the plugin recently, but I think he is a little busy elsewhere at the moment. I think any help here is welcome.
I noticed it as well and this is very disappointing. I think a goal should be that most ES modules converted to CJS by Rollup should be possible to convert back to ESM by this plugin, and this particular issue prevents that effectively. Myself, I am still working on the third part of the Rollup core changes, which unfortunately turns out to be a little bigger than anticipated and might take a few more days. Once done, I would switch over to this plugin but I would like to complete improvement 1 first to validate if the changes in Rollup make sense. Then I would be available for other improvements. If you would be willing to dig into improvement 2, that would be tremendous and should not cause conflicts with other work. If you need help, you can open a draft PR and we can continue discussion there. |
@lukastaegert About bug 3: I'm not sure it's a bug. Of course in terms of ES6 imports, if there's no Bug 4: Again, you said "definitely" but I'm not so sure about it :-) As if it was the other way around, exporting from CJS, you would treat the main Anyway, if anyone started working on this please share the branch so I could take a look. If not, I may pick up the gauntlet. |
Hi, no worries, I can feel this. Hope the move is worth it! With regard to your comments, Bug 3 is about being able to replace the CJS file with the ES6 file. You are right that it may not be considered a bug, but it also matches what other tools do, e.g. Babel: https://babeljs.io/repl/#?browsers=defaults%0A&build=&builtIns=false&spec=false&loose=false&code_lz=JYWwDg9gTgLgBAMwhRUIjgcgEYEMqYDcAUEA&debug=false&forceAllTransforms=false&shippedProposals=false&circleciRepo=&evaluate=false&fileSize=false&timeTravel=false&sourceType=module&lineWrap=true&presets=env%2Creact%2Cstage-2%2Cenv&prettier=false&targets=&version=7.10.4&externalPlugins= The point is, if the default export existed but was I somewhat agree with your argument for Bug 4 but here, Rollup definitely stands alone, and the behaviour here is not even compatible with Rollup's default behaviour where having a default and named exports, Rollup core exports the namespace. In any case, all other tools (Babel, TypeScript, Webpack) expect that requiring an es module exports the namespace, and there is a chance that Node will also go this way if they should ever support it, so, albeit slightly grudgingly, I think it is the way to move forward. Personally, I only worked on the Rollup core parts until now, but once this is done, I would like to start completing improvement 1 here to see if my changes in core work out before I release them. I will create a branch with a draft PR once I start. |
I think we need to clarify migration path to commonjs 14 for such packages. That changelog entry was not obvious for me as support for __esModule was dropped, not added. If there is no migration path what was the point in releasing the change? Currently my test environment is broken and I don't know should I downgrade or add hacks with manual |
Reopening as improvement 8 is still missing and also still scheduled to be fixed eventually. |
I've been updating my usage of rollup & @rollup/plugin-commonjs to the latest versions, and have been disappointed to find that not all the improvements mentioned in this issue seem to be working as advertised. My issues mainly seem centered around improvement 2. #817 - I filed the repro with reselect@3.0.1 but react-day-picker@7.4.8 has essentially the same issue. |
I have no idea who's at fault here but, complex projects that use
I.e. it looks like either P.S. rollup config import resolve from '@rollup/plugin-node-resolve'
import commonjs from '@rollup/plugin-commonjs'
import json from '@rollup/plugin-json'
import nodePolyfills from 'rollup-plugin-node-polyfills';
export default [
{
input: 'init/init.js',
output: {
file: '.init.js',
format: 'iife',
},
plugins: [
json(),
nodePolyfills(),
resolve({
mainFields: ['browser', 'module', 'jsnext', 'main'],
preferBuiltins: false,
}),
commonjs(),
],
},
] |
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. ⓘ |
This comment was marked as off-topic.
This comment was marked as off-topic.
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. ⓘ |
Expected Behavior / Situation
This plugin (and Rollup in extension) work as seamlessly as possible with output generated by itself and tools in the ecosystem.
Actual Behavior / Situation
There are a whole lot of interop issues, see details below. This is meant as an overview over how this plugin actually works and a more or less complete picture of all interop situations and the current behaviour. The idea is to use this as a base for discussion and to identify sub-issues that can be solved with reasonable effort. I will gladly try to take care of all changes to Rollup core but am very happy if I get support improving the plugin.
This list is VERY long, and I will try to update it based on suggestions. I will also try to add a compiled summary at the end soon.
As I strongly believe in testing over believing, I added actual output code samples for almost everything. To generate the pre-formatted code samples, I used the following
Rollup config
Importing CJS from CJS
The plugin needs to take care that this works seamlessly, resolving require statements with
module.exports
of the required module. This is not really an interop problem, the goal of this section is rather to highlight how the plugin actually works and handles certain scenarios and where it could be improved before moving on to the actual interop situations.Assignment to
module.exports
In the importing file, we generate two imports: An empty imports of the original module while the actual binding is imported from a proxy module. The reason for the empty import of the original file is to trigger loading and transforming the original file so that we know if it is CJS or ESM when building the proxy file.
The actual module renders two exports: What is assigned to
module.exports
is exported as bothdefault
and__moduleExports
. The proxy again exports__moduleExports
asdefault
(for situations where the proxy does slightly different things, look into the section where ESM is imported from CJS).Input
Transformed
Output
Assignments to properties of
exports
ormodule.exports
In this scenario, Rollup creates an artificial
module.exports
object that is created with all properties inline. This is very efficient as opposed to assigning the properties to an object one by one as the runtime engine can immediately optimize such an object for quick access. This object is again then exported as bothdefault
and__moduleExports
. Additionally, all assigned properties are also exported as named exports.Input
Transformed
Output
🐞 Bug 1: Assigning to the same property twice will generate two exports of the same name, causing Rollup to throw a "Duplicate export " error.
Handling unsupported use of
module.exports
orexports
There are a lot of cases where the plugin deoptimizes, e.g. when a property is read instead of assigned. In such situations,
createCommonjsModule
helper is used to create a wrapper to execute the module more or less like Node would execute it without detecting any named exports.Input
Transformed
Output
Inline
require
callsAt the moment, the plugin is NOT capable of maintaining exact execution order. Rather, even nested and conditionally executed require statements (unless they are written via an if-statement in a particular way) are always hoisted to the top. This is a separate situation that could be improved by drastic changes, i.e. wrapping modules in function enclosures and calling them when they are first used. This will however have a negative effect on the efficiency of the generated code as compared to the status quo so this should only happen when it is really necessary. Unfortunately, it is not possible to tell if a module is required in a non-standard way until the whole module graph is built so in the worst case, all modules might need to be wrapped. Rollup could help here a little by implementing some inlining algorithm, however this is very much future talk (say, up to a year in the future) and will likely only apply anyway if a module is used exactly in one place. Other approaches could be for the plugin to analyze the actual execution order to see if it can ensure that the first usage does not need wrapping so that it does not matter if there are dynamic requires later on but this feels complicated and error-prone.
Anyway, this is mostly listed for completeness here as it does not really touch on the subject of interop but warrants its own issue. Here is a sample to illustrate:
Input
Transformed
Output
Now let us get to the actual interop patterns:
Importing CJS from ESM
NodeJS style
In Node, CJS modules only expose a default export that corresponds to
module.exports
. This important pattern should always work.For the plugin, the main difference here is that instead of the proxy, the actual module is imported directly. Here just one example to illustrate, in general everything works similar to the CJS-to-CJS case.
Input
Transformed
Output
NodeJS style with named imports
This is supported by Webpack but (previously in part but now fully) also by this plugin. In addition to the default import resolving to
module.exports
, named imports will resolve to properties onmodule.exports
. Previously, this would only work for named exports that the plugin could auto-detect (and only if the module was not deoptimized to usecreateCommonjsModule
) or which were listed by the user. The use of Rollup'ssyntheticNamedExports
property in its current form now enables arbitrary named imports to be resolved without additional configuration while even maintaining live-bindings.A Note about Webpack .mjs semantics and better NodeJS interop
Note that Webpack currently either disallows or warns about this pattern when used from modules with the
.mjs
extension.🚧 TODO: Would be nice to confirm this
The intention here is that this extension signals we want to enter some sort of strict NodeJS interop mode. We could do something similar, but then I would love to have the ESM/CJS detection in
@rollup/plugin-node-resolve
and establish a communication channel to get this information from that plugin. Then we might add a switch to@rollup/plugin-commonjs
to use "strict NodeJS interop" thatThis could become an advanced feature we add after solving the more pressing issues we have at the moment.
Example with statically detected named exports
Input
Transformed
Output
Example that relies on synthetic named exports
Here we just assign an object to
module.exports
. Note how we retain live-bindings by using property accesses: If the object atmodule.exports
would be mutated later on, accessing our named variable would always provide the current value.Input
Transformed
Output
ESM that was transpiled to CJS
This is the tricky one. The problem here is to maintain isomorphic behaviour between the original ES module and the CJS module. Named exports are handled mostly correctly when using the "NodeJS with named imports" pattern (except we do not throw for missing exports) however the default export should not be
module.exports
butmodule.exports.default
. This is incompatible with the previously listed interop patterns.At the moment most tools implement a runtime detection pattern for this by adding an
__esModule
property tomodule.exports
to signify this is a transpiled ES module. Then the algorithm when getting the default import ismodule.exports.default
as the default exportmodule.exports
Example importing a named export when a default export is present
🐞 Bug 2: This is not working correctly as instead of the named export, it tries to return a property on the default export. The reason is that in this situation, the interop pattern in
unwrapExports
kind of correctly extracts the default export and exports it as default, butsyntheticNamedExports
should not use that to extract named exports.Input
Transformed
Output
This is quite difficult to fix, especially if we want to maintain live bindings for named exports. My first thought was to extend
syntheticNamedExports
to allow an additional value to indicate that the default export is also picked asdefault
property from the actual default export. This would mean however that auto-detecting interop becomes slow and difficult and destroys likely all live-bindings for the non ESM case because we need to build a new object, i.e.📈 Improvement 1: A better idea I had is to allow specifying an arbitrary string as value of
syntheticNamedExports
, e.g.syntheticNamedExports: "__moduleExports"
. The meaning would be that missing named (and even the default) exports are no taken from the default export but from a named export of the given name. Then the interop would just beThis is rather efficient, though it could still be put into an interop function
getDefault
if we want to save a few bytes. Of course we still do not get live-bindings for thedefault
export in the transpiled ESM case, but even this is fixable if in a second step, we implement static detection for__esModule
:📈 Improvement 2: If we come across the
Object.defineProperty(exports, "__esModule", { value: true })
line (or!0
instead oftrue
for the minified case) on the top level of a module, then we can just mark this module as being transpiled and can even get rid of this line in the transformer, making the code more efficient and removing the need for any interop, i.e. above we do not add theexport default
at all in that case. There is also no longer a need to wrap the code increateCommonjsModule
if this property definition is ignored.Example importing a non-existing default export
🐞 Bug 3: It is not really surprising that this case is not working correctly as it should actually throw either at build or at runtime. Otherwise at the very least the default export should be
undefined
while here it is actually the namespace.Input
Transformed
Output
To fix this, I would like to reduce the interop pattern to only look for the presence of
__esModule
and nothing else. This would be covered by the suggested Improvement 2.Importing ESM from CJS
To my knowledge, there is no engine that supports this at the moment, so the question is what the "correct" return value should be. For NodeJS, I heard that there is actually some interest in supporting this eventually but the road blocks are technical here (mostly that the ESM loader is async + TLA handling and similar things). However if this is ever supported, a
require
of an ES module will likely return the namespace. Otherwise Webpack supports this of course, and to my knowledge here you always get the namespace as well. Looking at TypeScript it will always add the__esModule
property on transpiled ESM modules and use thedefault
as a property which in turn means that if yourequire
an ES module and use CJS as output target, you always get the namespace. The same goes for Babel.For Rollup, it is more complicated, and the reason is in part rooted in that fact that Rollup was first and foremost designed for libraries, and here you want to be able to create CJS output where everything, e.g. a function, is directly assigned to
module.exports
to be able to create a nice interface for "one-trick-pony" libraries. So Rollup core by default uses its "auto" mode when generating CJS output, which meansmodule.exports
contains the namespace unlessmodule.exports
Now of course this is about converting ESM to CJS but one idea is that in such a situation, the CJS and ESM versions should be interchangeable when I
require
them. So for internal imports, it could make sense to work like Rollup's auto mode in reverse, giving you the default export when there are no named exports and the namespace otherwise. But I understand that in the long term, we should likely align with the rest of the tooling world even if it generates less efficient code, so my suggestion on this front is:require
by defaultexternal
option in Rollup works) to work like auto mode as outlined above to make existing mixed ESM CJS code-bases work. The reason I think we need this is that the requiring module can easily be a third party dependency itself and thus not under your direct control.Requiring ESM with only named exports
This works as intended.
Input
Transformed
Output
Requiring ESM with only a default export
This one is working correctly for auto mode but with the arguments above, it should likely be changed, see Improvement 3 and Improvement 4.
Input
Transformed
Output
Requiring ESM with mixed exports
🐞 Bug 4: This one is broken—it should definitely return the namespace here but instead it returns the default export.
Input
Transformed
Output
External imports
To view the full interop story here, one has to look both at what this plugin generates and what Rollup generates as CJS output.
External imports in the CommonJS plugin
🐞 Bug 5: For external imports, this plugin will always require the default import.
📈 Improvement 6: With the arguments given above, this should actually be the namespace by default (
import * as external from 'external'
) as this is technically equivalent to requiring an ES module.📈 Improvement 4b: Again we should add an option to specify when an external require should just return the default export instead. It could even be the same option. So here is some room for discussion.
Input
Transformed
Output
External imports in Rollup's CJS output
Importing a namespace
Ideally, this should be converted to a simple
require
statement asrequire
would again become a simplerequire
.And this is indeed the case:
Input
Output
Importing named bindings
These should just be converted to properties on whatever
require
returns as that should be equivalent to a namespace. And that is again the case:Input
Output
Importing the default export
🐞 Bug 6: Several things are broken here:
default
property instead of checking the__esModule
property. I always thought there was a reason buried in some old issue but going back in history it seems it has always been that way. This should change as it has all sorts of adverse effects: If the file was not an ES module but assigns something toexports.default
, it will be mistaken for a default export; if it was an ES module but does not have a default export, this will wrongly return the namespace instead.Input
Output
📈 Improvement 7: To fix this, I think I would try to go for how Babel does it.
Entry points
Entry point which assigns to
module.exports
This will be turned into a default export which is probably the best we can hope for.
Input
Transformed
Output
Entry point which adds properties to
exports
This appears to be working sensibly.
Input
Transformed
Output
Entry point which assigns to
module.exports
but requires itself🐞 Bug 7: Basically it looks for a
__moduleExports
export that does not exist instead of giving the namespace. My suggestion above for how to reworksyntheticNamedExports
in Improvement 1 should also fix this from within Rollup. Similar problems arise when another module imports our entry point or when the module just adds properties toexports
.Input
Transformed
Output
Entry point which assigns to
exports.default
🐞 Bug 8: Here no output is generated while it should likely default export the namespace unless the
__esModule
property is present.Input
Transformed
Output
// "bundle.js"
Entry point that is a transpiled ES module
Ideally, the output should have the same exports as the original entry. At the moment, this will not be the case as the
Object.defineProperty
call will always cause thecreateCommonjsModule
wrapper to be used and no named exports will be detected. There are several ways to improve this:__esModule
property definition and do not treat it as a reason for deoptimization, see Improvement 2namedExports
that specifically lists exposed exports for entries. We could also use this to activate or deactive the default export and decide if the default export should be whatever is assigned toexports.default
or rathermodule.exports
. This could be handled by simply creating a wrapper file that reexports these exports. If we do it that way, Rollup tree-shaking might even remove some unused export code.Input
Transformed
Output
Summary and possible action plan
In Rollup:
I will gladly take care of the necessary improvements here:
auto
mode without specifying it explicitly and there is a package that has only a default export. Explain how this package will not be interchangeable with its ESM version in many tools and suggest to use named exports mode with all its consequences or better, do not use default exports.► Warn when implicitly using default export mode rollup#3659 ✅
syntheticNamedExports
should support receiving a string value that corresponds to an export name from which to pick missing exports. The value oftrue
would correspond to"default"
except that for entry points when using a string, the listed property name would not be part of the public interface. This will fix issues where suddenly__moduleExports
is introduced into an interface.► Add basic support for using a non-default export for syntheticNamedExports rollup#3657 ✅
► Rework interop handling rollup#3710 ✅
In this plugin:
Implement Improvement 3, Improvement 4a, Improvement 4b and Improvement 6: Always return the namespace when requiring an ES module except when a particular option is set. What is returned is configured in the proxy module. At the moment, we only return the namespace if the module does not use a default export. There are many ways how such an option might be done, here is one suggestion:
name:
requireReturnsDefault
supported values:
false
(the default): All ES modules return their namespace when required. External imports are rendered asimport * as external from 'external'
without any interop.true
: All ES modules that have a default export should return their default when required. Only if there is no default export, the namespace is used. This would be like the current behaviour. For external imports, the following interop pattern is used:It might make sense to extract this into a helper function that is reused where possible.
"auto"
: All modules return their namespace if required unless they have only a default export, in which case that one is returned. This is the inverse of Rollup's auto mode. For external imports, the following interop pattern is used:Again it might make sense to extract this into a helper function.
this.resolve
so that the user can just use the name of a node_module. In that case if the module is not an ES module or does not have a default export, the plugin should throw, explaining the error. External imports are just rendered asimport external from 'external'
without any interop.true
orfalse
|undefined
. It should be ensured that this function is only called once per resolved module id. This is equivalent to specifying an array of all module ids for which we returnedtrue
.► Return the namespace by default when requiring ESM #507 ✅
Once it is done on Rollup core side, implement the plugin part of Improvement 1: Use the new value for
syntheticNamedExports
and add the default export corresponding to the suggested simplified interop pattern.► fix(commonjs): fix interop when importing CJS that is a transpiled ES module from an actual ES module #501 ✅
Implement Improvement 2: If a property definition for
__esModule
is encountered, remove it, do not treat it as a cause for using thecreateCommonjsModule
wrapper and do not add the interop default export to the module. Ideally, in this scenario, assignments toexports.default
should be treated like assignments toexports.foo
in that it generates an explicit export. So this:should be converted to
► feat(commonjs): reconstruct real es module from __esModule marker #537 ✅
Implement Improvement 8: Allow specifying exposed exports. My suggestion is:
name:
exposedExports
value: An object where keys correspond to module ids. For convenience, the keys should be put through
this.resolve
by the plugin. The value is an array of named exports. Internally, this could just mean we resolve the module not to itself but rather to a reexport file, e.g.The new
' \0my-module?commonjsEntry'
would correspond to that we usually render here. Commonjs proxies should also import from that file.The text was updated successfully, but these errors were encountered: