Skip to content
This repository has been archived by the owner on Aug 4, 2021. It is now read-only.

Add support for transpiled modules #16

Merged
merged 3 commits into from
Dec 24, 2015
Merged

Conversation

Victorystick
Copy link
Contributor

Don't merge yet

This PR tracks how best to handle CommonJS modules that have already been transpiled by Babel, TypeScript or something similar. These CommonJS modules define a __esModule property on their exports object, and export their default export as a default property.

We should:

  1. remove the __esModule property
  2. use the default property for the default export instead of the module.exports object

Fixes #10

@Rich-Harris
Copy link
Contributor

Would it be appropriate to warn users in this situation? It may be that we're dealing with external packages, in which case the user may have limited control, but it might help people understand that their Babel config is messed up etc

@flying-sheep
Copy link

messed up? does this mean it’s somehow not working as intended? why?

@Rich-Harris
Copy link
Contributor

@flying-sheep Messed up as in Babel is being used to convert ES6 modules to CommonJS modules, which means Rollup has to convert them back to ES6 modules so that it can bundle them (quite possibly in order to generate a CommonJS bundle). Keeping everything as ES6 until you need to run it is the name of the game.

@flying-sheep
Copy link

ok, but how? jsnext:main should point to a “distributable” package, not a ES2015 package… should we really introduce another field and copy of the lib for an ES2015 version?

i’ll comment in rollup/rollup#337 about it.

for this issue and plugin, i think we should simply detect the most common ways stuff is distributed and be practical instead of idealistic. this is a practical plugin, written for a world of commonjs modules. it should “get” as many of them as possible.

@Rich-Harris
Copy link
Contributor

Should have been more specific – I meant keeping everything as ES6 modules is the goal, since that's the only thing that's getting translated back and forth otherwise.

@flying-sheep
Copy link

makes sense. this is a frequent language confusion in the discussions here.

but still: both should be supported as we can’t hope to convince everyone to use jsnext:main (which means adding build configs and doubling package size) and therefore should be practical.

  1. let’s help redux with fixing their use of jsnext:main
  2. let’s finish and pull this PR

then we can actually work productively with rollup as node_modules bundler

@Victorystick
Copy link
Contributor Author

Looks good to me. 👍

Rich-Harris added a commit that referenced this pull request Dec 24, 2015
@Rich-Harris Rich-Harris merged commit 25e4cf0 into master Dec 24, 2015
@Rich-Harris Rich-Harris deleted the handle-transpiled-modules branch December 24, 2015 22:08
@Rich-Harris
Copy link
Contributor

Have released this as 2.0.0 (since it's technically a breaking change) – it doesn't include warnings about already-transpiled modules, but we could revisit that in future if we choose

@fregante
Copy link

Sorry, I think this change is indeed breaking support of rollup modules in browserify/babel. If you export default but not __esModule, browserify/babel won't pick that up. I had left a comment here too: rollup/rollup#496 (comment)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants