-
Notifications
You must be signed in to change notification settings - Fork 116
Test proposals #2
Comments
Thanks – this definitely does need more tests. At the moment it's a very basic proof-of-concept. Sadly I don't think named imports/exports are realistic. It's just way too hard to cover all the many ways in which a CommonJS package author could foil whatever techniques we used to identify them: var myPackage = exports;
myPackage.foo = function () {...}
function augment ( pkg ) {
pkg.bar = function () {...}
}
augment( myPackage ); We couldn't turn that into named Fundamentally, a CommonJS module is only capable of exporting a single default object, much as it likes to pretend otherwise. (Not a criticism of CommonJS, that's just how it is – it took a change in the language to improve upon it.) So, given that we'd fail to handle those cases (both of which I've seen in the wild) well, we should limit ourselves to default exports. It might be confusing for people who do... import { named } from 'some-commonjs-module' ...but I think we can solve that with good error messages and documentation. |
This leaves me grumpy. On one hand you're absolutely right. On the other I'm wondering if we can do a little bit of magic to enable rollup savings between most modules. I could see the "cleverness risk" of named exports drop pretty low for the case of a direct assignment or property only assignment of And the only way to actually make use of the named exports is to know which exports we were able to name and the modify the imports accordingly. I'm willing to bet this would cover a large % of export statements with great success. |
Somehow we managed to implement this without closing this issue, at least on the exporter side – named exports are created alongside default exports where possible. We're not creating named imports, but there isn't really anything to be gained by doing so anyway |
I know it's probably early for this but I worked up the test cases for imports and exports of how I think CommonJS to es modules might look.
https://gist.github.com/reconbot/5358ab4a532d2f253df0
I think this would cover all commonJS use cases but I'm not sure. And the last import example is a nice to have.
The text was updated successfully, but these errors were encountered: