-
-
Notifications
You must be signed in to change notification settings - Fork 585
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 Node.js Default Interop Case #635
Comments
/cc @weswigham it would be really interesting to hear your perspective here from the TypeScript team in terms of where Node.js interop meets bundlers and ways of tackling this default interop alignment. |
I would agree that maximal alignment would be great so that we can get a uniform ecosystem and not have people include feature detection code within libraries. |
The choice seems fairly clear cut - the purpose of I see standards compliance, or in this case "preservation of platform semantics", as one of the reasons why folk choose Rollup today. Maybe you could even consider it a differentiator. So let's retain that. |
I think this is a very valuable discussion to have. Also worth noting: Webpack is more likely to either pick other semantics in the future or have flags that change them. Node.js (because it needs to keep running existing code) is much more hesitant to break code that may depend on the current |
While it is certainly prudent to align with Node, and this might even become a default, I am convinced we should keep the existing interop behaviour at least behind an option. TypeScripts Talking about TypeScript, their behaviour without interop seems rather inconsistent to me: You would expect that a namespace import converted to CommonJS would return an object where the Babel, on the other hand, goes all-in on the I hope there is also discussion with them to align with the Node behaviour? Also to be clear, a lot of the changes in the v15 of the CommonJS plugin were not triggered by incompatibilities with Webpack alone, but rather by inconsistencies with TypeScript. At the moment, this is a whole ecosystem vs. the Node way, and while there are good arguments for adopting the Node way, we will just make the interop situation even worse if major players like Babel are not on board. |
Thanks for the thoughtful feedback here. Agreed an option might well make sense. When talking about Babel, I think the important distinction to consider is that there is a difference between a CommonJS module with an So the difference is that if you are in a CommonJS "faux module" and you load another CommonJS "faux module", then I think it is expected that you get a different interop from when you are in an ES module (real module!), and loading a CommonJS "faux module". The desire to have this interop match the faux module interop is what is causing the difficulties here. Babel has nothing to do with this case though. Certainly TypeScript alignment is important, and that's why I copied in some TypeScript people here. I do hope we could get more feedback on how they hope to handle these cases and I agree that RollupJS should not have to make these kinds of decisions as a project alone. |
When I create a project as ESM that has an external dependency "foo" which is faux ESM, and I convert my project to CommonJS via Babel, I will very much get the wrong behaviour. Not sure there is anything they can do to fix this, though. In the end, converting a project with external dependencies via Babel will just never be compatible with Node I guess. |
Again reiterating the edge case aspect, named exports work out as expected here. Let's flesh out your example for the edge case that breaks. Say we have a Babel faux module like: exports.__esModule = true;
exports.default = 'default'; To import this module in Node.js, we need to do a double default access: import m from 'cjs';
const val = m.default;
console.log(val);
// outputs: 'default' We then convert this module into CommonJS: const _cjs = require('cjs');
const m = (_cjs.__esModule ? _cjs : { default: _cjs }).default;
const val = m.default;
console.log(val);
// outputs: undefined The value of the Now I agree that ideally Node.js would have just used the In terms of the Babel output it is correct. There is no action for Babel to take here. The fix in this case is relatively straightforward for the user though. They control the code as they are the ones doing the transpilation here in the first place. So they are in a position to fix the problem. You do some debug logging and you notice that the value isn't the default property anymore, so you correct the code with something like: import m from 'cjs';
const val = (m.default || m);
console.log(val);
// outputs: 'default' in order to get something to work in both faux ESM <-> fauxESM and ESM <-> faux ESM modes. Given that this interop does already vary between environments, having the above is likely advised anyway for users if they want their code to work. It's important to remember that as soon as next year when Node.js 10 LTS ends it's no longer necessary to compile code into CommonJS. On the other hand we will still be importing CommonJS into real ES modules in the ecosystem for many many years! So in terms of the calculus, I do think the argument for Node.js interop outweights a probem of the order of a year that can be corrected. |
Personally, I don't think it's reasonable to write a module that's compatible with both being able to compile to the faux esm the tool chain is used to and run under node's real esm at present - there are enough gotchyas and edge case differences that attempting to do so requires a wealth of specialized knowledge the average user is unlikely to have, and you usually still have small differences in behavior that could end up affecting someone down the line. I, personally, am of the opinion that one should either be using node esm, or transpiled esm, but not both in the same codebase. Likewise, tools like roll-up should probably be able to figure out which was a package's source's intent (likely via export maps - if an esm module is found via node |
My 2 cents. From an user perspective, CJS-ESM interop is hard. With the Node.js interop, when importing a dependency I have to check:
(1) and (2) make sense, (3) does not. I wish Node.js supported Bundlers could try to make it more ergonomic, by checking if there is an
That is, one more rule that I need to be aware of. Having an exception ( (3) ) already imposes a cognitive load, but having an exception that is only an exception sometimes is probably worse. Rollup should follow Node.js' interop strategy, even if it's sub-optimal in some cases. |
As a user, having to do |
I wonder if rollup(plugin-commonjs)could as well align its default CommonJS detection algorithm with NodeJS, without inspecting file content, at least for modules under
|
@shrinktofit discussion on this should really be a separate issue, but to mention briefly - "type": "module" can be useful to indicate JS files that don't use import or export syntax that should still be parsed as ES modules, but disallowing ".js" files that use import or export syntax within a non "type": "module" boundary is likely unnecessary friction as they wouldn't execute in Node.js so the subset of Node.js semantics that execute correctly is contained within the RollupJS semantics then fine. |
One thing to note: However there is an "interop" pattern, or rather a conversion pattern, that allows to convert default exports to something that is compatible with Node, which is to convert export default 'foo'; to module.exports = 'foo'; This has some downsides:
As for the latter, I think everyone will agree by now that in such a situation, you should convert everything to named exports if you want to have any sane chance of working interop. But for library authors, having the opportunity to specify only a default export is important. This interop pattern is actually what Rollup core has been using as default for years, and I was only now considering to change it to align more with the rest, i.e. Babel and Webpack. But the recent discussions make me feel that we should actually double down on this old pattern. This would not be something for this plugin but mostly for Rollup core, which is very capable of finding out if there are potential circular dependencies or live-binding issues and can warn about this.
That is just not true, unless you want to maintain a mostly CommonJS code-base. Because the only "safe" solution for the dual-state hazard still remains to have an ESM wrapper around a CommonJS core (there is also the alternative of JSON modules, but there is still danger from assuming reference identities). So if you want to author your library as ESM only in the hope of eventually getting rid of the CommonJS parts, you need something like Rollup for the shared core. A last point I want to add: If a module contains the Adding such a warning would not be a big issue, but I think it would be fundamentally important. |
@nicolo-ribaudo perhaps Babel can bring this back as a custom option? I recall the reason for changing this was that adding named exports becomes an API break, but perhaps the UX here can be reconsidered a bit.
I definitely agree that in general discouraging named exports and default exports together for ESM -> CJS outputs specifically seems like the best chance of glossing over interop issues. They are fine for internal dependencies etc etc, but just not for the outputted entrypoint module. Similarly in the modules RollupJS emits. If we can get to a place where no __esModule CommonJS modules have a default export then that is a good place to be!
Sensible warnings sounds like a great way to start to work through the user experience issues here before users hit the compatibility cases. I would still like to push for the Node.js interop in due course if possible, and may even work on a PR at some point if you are not personally interested in this. Hopefully we can continue to iron out these cases in the mean time. I brought up the same discussion to @evanw on esbuild today here - evanw/esbuild#532. Seems like compat discussions between bundlers will be quite important going forward. |
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. ⓘ |
I'm trying to add a new option to Babel which makes it reflect the Node.js behaviour: babel/babel#12838 Without that option, it's impossible to easily write ESM code which works both natively and when compiled: until that Babel option is merged, we have to rely on a custom plugin (babel/babel#12795). I think this issue should be reopened, to consider a similar option for Rollup. |
Thanks for following up here @nicolo-ribaudo, reopening. |
Fyi, I opened #838 to continue this discussion with an actual implementation because we accidentally introduced a bug in Babel because of the difference between Node.js and Rollup. |
Feature Use Case
This is an edge case of the Node.js ESM / CJS interop.
The Node.js ESM / CJS interop semantics are described in the following section of the documentation - https://nodejs.org/dist/latest-v15.x/docs/api/esm.html#esm_commonjs_namespaces.
Up until version 14, RollupJS actually exactly followed these Node.js semantics (unintentionally yes, but it did nonetheless!).
A good test example is the following:
main.js
dep.js
Running:
For version 14 of the plugin we get the output:
While for version 15 and up we get the output:
That is, it was only as recently as this August that RollupJS started diverging from what is considered the Node.js interop at this point.
Feature Proposal
I would like to propose that RollupJS reconsider the default behaviour here and consider aligning with Node.js.
Webpack is specifically not going with Node.js alignment on this edge cases, but RollupJS still can.
I think this kind of thing is important to get good behaviours universally in the ecosystem moving between environments. And I don't think blindly following Webpack is the recipe for success necessarily even though it is the most tempting path due to the majority of the ecosystem expectations being built there.
I understand this is a difficult discussion though so the first thing to do is to remember that this is an edge case we are discussing. It already affects a minority of cases.
But these small edge cases are what define the development experience when working with JavaScript for many developers.
If we can improve these friction points and make the interops align then we do well by the ecosystem.
There is a decision here to be made in either aligning with Webpack or Node.js here. I'm pushing for the Node.js side, because I feel that as a platform it has a long future and that Node.js build workflows should be considered first class.
Since this behaviour has only been in the plugin for two months I don't think it unreasonable to consider making a breaking change to support this sort of thing, and it would be a step made by RollupJS as a project to improve future interop compatibiity.
Further discussion / questions / feedback on this topic very welcome.
//cc @LarsDenBakker @lukastaegert @shellscape
The text was updated successfully, but these errors were encountered: