This repository was archived by the owner on Aug 4, 2021. It is now read-only.
WIP: more complete CJS <-> ES interop #91
Closed
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Via rollup/rollup#866. Interop between CommonJS and ES modules continues to be a total bloody nightmare. I think our current approach is incomplete but I'm not sure what the best one is. Stream of consciousness ahead as I try to get my thoughts straight.
There are three situations this plugin needs to handle: ES imports CJS, CJS imports CJS, CJS imports ES.
Right now there's interop fudging at export time and at
requiretime. It causes problems when CJS files require other CJS files that were generated by a transpiler, as is the case in rollup/rollup#866 – react-apollo/index.js does things like this:This plugin borks things up because it massages the exports of
ApolloProvideron the assumption that the importer will be an ES module, not another CJS module.Taking each situation in turn:
ES imports CJS
This case is relatively straightforward –
foo.jsjust exports the value ofmodule.exports(ormodule.exports.defaultif it's a transpiler-generated module – tbh we should probably only do that ifexports.__esModule === true, because that's how transpilers mark their territory).bar.jshas a namedbarexport in addition (either automatically, because the plugin detects anexports.bar = whatever' assignment, or manually because we've usedoptions.namedExports.CJS imports CJS
It seems fair to say that in this situation, no export massaging should occur –
xshould simply be the value ofmodule.exportsinfoo.js. That's not what currently happens, meaning that when a CommonJS module expects to find a.defaultproperty on a required module, things blow up.Unfortunately, there's no (robust, reliable) way for this plugin transformer to know whether its consumer will be ES or CJS (or both!).
CJS imports ES
Because of this requirement, all
requirestatements result in namespace imports rather than default imports – since we don't know ifbar.jsexportsdefaultor not, we have to import a namespace and futz around with it. This is really bad news, because it results in more code and complexity and prevents tree-shaking.The approach in 61c6e00 fixes rollup/rollup#866 but in a horrible way – by adding a
.defaultproperty to the.defaultproperty, so that an importer of the transformed module will find what it expects whether it's ES or CJS. (It doesn't currently hang any named properties off the default export, so isn't a complete solution in any case.)There has to be a better way. I'm wondering about something involving virtual 'proxy' modules that do the interop stuff – i.e. an ES module imports the transformed module directly, whereas a CJS module imports the proxy. Or vice versa! Though my thinking hasn't progressed far beyond that right now.
If anyone can see anything obvious I'm missing let me know... you might save me from being driven insane by this stuff.