-
Notifications
You must be signed in to change notification settings - Fork 126
Conversation
@TrySound don't suppose you have any insight into why this is failing on Windows?! |
Ah, shit. This won't work in its current form. We can't use 'virtual modules' because they get disregarded by other plugins – so code isn't transformed etc. Back to the drawing board (╯°□°)╯︵ ┻━┻ |
Okay, back in business. I've flipped things around such that the 'real module' contains the actual module contents, while the 'virtual module' ( We have to jump through some slightly ugly hoops in order to prevent the entry module (if it's CommonJS) exporting This now plays nicely with other plugins, and no longer breaks sourcemap support. Now if we could just figure out why this isn't building on Windows... |
I'll check it out home. |
idea: only wrap the commonjs modules if there is a top level return |
Is top level return valid for commonjs modules? |
@calvinmetcalf yeah, I've been thinking about this sort of thing – ideally we would be able to turn this... module.exports = 42; ...into this... export default 42; ..and this... exports.foo = 'bar'; ...into this... export var foo = 'bar'; ...but that's separate to this issue, which is purely about interop. We'll need to revisit it separately. (Also, I don't want to spend too much time reducing the incentives for CommonJS holdouts to join us in 2016 already...) |
ah I was more thinking if your doing a big rewrite, now would be the time On Wed, Aug 31, 2016 at 11:26 AM Rich Harris notifications@github.com
|
yes On Wed, Aug 31, 2016 at 11:56 AM Bogdan Chadkin notifications@github.com
|
} | ||
}) | ||
.join( '\n' ); | ||
transformBundle ( code ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like dead and expensive code.
Awesome, thanks @TrySound. Have released this is 4.0.0.
in my experience a big rewrite is exactly the wrong time to change the behaviour 😀 Better to lock in the bug fixes and modernise the codebase etc then tackle the wishlist. It should actually be a little bit easier to get this stuff working with the current design – I'm thinking we try to convert it the nice way, then bug out and fall back to the current |
Just FYI. I'm using this on a project with both react and preact. Simply upgrading this plugin made the gzip bundle smaller. 68B saved when using preact, 6kB when using react. The reason why there's a big saving with react is because some useless (hope so) exported modules disappeared. Thanks |
@piuccio awesome! thanks for sharing those numbers. Should fall even further once we can transform CJS modules without wrapping them in |
Glad to know that more bytes can be saved. For correctness I should mention that the savings on react are due mostly to #93, while the savings on preact can certainly be attributed to this PR. Thanks for the great work. |
This supersedes #91. Summary:
module.exports
, or, ifexports.__esModule === true
,exports.default
(previously it would always use.default
if it was present).default
interop no longer takes place – this fixes bugs like React ApolloProvider component is undefined rollup#866I won't lie: the approach here feels a little bit crazy. I think that's just the price of having good (and efficient) interop.
Before the explanation, a couple of comparisons:
Basic CommonJS importing
Source
Before
After
Inline require statements
Source
Before
After
How it works
Basically, when you import a CommonJS module from an ES module, you're no longer importing the module itself but a proxy, which imports the actual CommonJS module but with a prefixed ID (
\0commonjs-required:/path/to/cjs-module.js
). The proxy handles the ES <-> CJS interop – deciding whethermodule.exports
orexports.default
should be the default export, and adding named exports – while the module itself (with the prefixed ID) always just has a single default export which ismodule.exports
. Its dependencies are re-declared as prefixed imports.Meanwhile, if a module with a prefixed ID (i.e., imported by a proxy or a CommonJS module) isn't a CommonJS module, we need a different kind of interop layer, which imports the underlying ES module and exports-as-default either its default export (if there is one) or the entire namespace. That way, we don't need to reify namespaces for everything that a CommonJS module imports, only ES modules.
An unfortunate side-effect of this is that sourcemaps no longer work for CommonJS modules, because the source code lives in virtual modules, which get excluded. I haven't been able to think of a good solution to this. Honestly, I think it's a small trade-off for more reliable interop and more efficient code, given that sourcemaps are mostly useful when you're debugging your own code.
I think this covers all the bases. @rollup/collaborators and others – would welcome any feedback on this before we commit to this road! Sorry for the rambly and complex explanation.