Skip to content
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

use an interopDefault function for external CJS deps #458

Merged
merged 2 commits into from
Jan 21, 2016

Conversation

aearly
Copy link
Contributor

@aearly aearly commented Jan 20, 2016

Closes #419 . Also makes async's modular builds a bit smaller. :)

@Victorystick
Copy link
Contributor

Good idea. To ensure that we don't get any name collisions, we should probably add _interopRequire to the list of expected global names here. That'll ensure that any declarations like var _interopRequire will be automatically renamed. We don't want to have our _interopRequire function inadvertently overridden.

$ = 'default' in $ ? $['default'] : $;
function _interopRequire (id) { var ex = require(id); return 'default' in ex ? ex['default'] : ex; }

var $ = _interopRequire('jquery');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be done in two steps like it was before, otherwise using browserify or the like on the output won't really work. Unless I'm missing something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is, there should still be a require('jquery') for such tools to find.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i.e.

function _interopRequire (mod) { return 'default' in mod ? mod['default'] : mod; }
var $ = _interopRequire(require('jquery'));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right -- it'll have to be a two-step process.

@Victorystick
Copy link
Contributor

I agree with @eventualbuddha.

@aearly Also just noticed that the alphabet test is broken after these changes. The current behaviour

var alphabet = require('alphabet');
var alphabet__default = 'default' in alphabet ? alphabet['default'] : alphabet;

This PR

// alphabet equals alphabet__default, although it shouldn't
var alphabet = _interopRequire('alphabet');
var alphabet__default = alphabet;

What we want is to extract the 'default' in alphabet ? alphabet['default'] : alphabet into a common function, and execute that on the whole import.

var alphabet = require('alphabet');
var alphabet__default = _interopRequire(alphabet);

@aearly
Copy link
Contributor Author

aearly commented Jan 20, 2016

One other thing -- we can also do something similar for the AMD/UMD/IIFE outputs too by modifying finalisers/shared/getInteropBlock.js. I wanted to get the CJS output working first.

@aearly aearly changed the title use an interopRequire function for external CJS deps use an interopDefault function for external CJS deps Jan 20, 2016
Rich-Harris added a commit that referenced this pull request Jan 21, 2016
use an interopDefault function for external CJS deps
@Rich-Harris Rich-Harris merged commit 88a0e91 into rollup:master Jan 21, 2016
@Rich-Harris
Copy link
Contributor

awesome, thanks @aearly!

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

Successfully merging this pull request may close these issues.

None yet

4 participants