Problems with external dependencies

[Rich-Harris]

Found while adding a test for #57 – the ES6 output should be identical to the input in this scenario:

// main.js
import factory from 'factory';
factory( null );

Instead we get this:

factory( null );

[Rich-Harris]

Changed the title of this issue since there are a couple of different things going wrong. Working on fixes

[Victorystick]

I've also noticed this problem. I haven't pushed anything yet, but I will tomorrow. If nothing else, we can compare notes.

[Rich-Harris]

Just pushed https://github.com/rollup/rollup/tree/gh-59 but have to run now - most stuff working but still a couple of problems with ES6 output

[Victorystick]

Please take a look at some changes to gh-59. Fixes the issue of ES6's import * as ns from '..' in particular.

[Victorystick]

This issue also made me think about the UMD and IIFE formats, and how they manage import names. Say I want to use Albatross (random library name), like so:

import A from 'albatross';
A.someMethod('...');

// Yields the IIFE:

(function (A) { 'use strict';

    A.someMethod('...');

})(A); // <-- Assumed name of Albatross global.

Albatross is assumed to be exposed through a global named exactly A, the same as the import name. This seems really fragile. Looking into rollups predecessors revealed JsModuleTranspiler by Katz. In one part of the README, his solution is described. Users supply a map from import names (in the example albatross) to global name (Albatross).

This problem is made more obvious by the test extensions.

import * as containers from 'shipping-port';
...

// Yields the IIFE:

(function (containers) { 'use strict';

    ...

})(containers); // <-- That doesn't seem right!

[Rich-Harris]

Nice - have merged your changes into gh-59. I think the only remaining problem here is imported bindings being erroneously renamed in ES6 bundles.

Re the Albatross example - this is in fact supported, it's the docs that are missing 😬 You just need to specify globals, like so:

import $ from 'jquery';

$( function () {
  $( 'body' ).html( '<h1>hello world!</h1>' );
});

rollup.rollup( opts ).then( bundle => {
  const result = bundle.generate({
    globals: { 'jquery': 'jQuery' },
    format: 'umd'
  });
});

Result:

(function (global, factory) {
  typeof exports === 'object' && typeof module !== 'undefined' ? factory(require('jquery')) :
  typeof define === 'function' && define.amd ? define(['jquery'], factory) :
  factory(global.jQuery);
}(this, function ($) { 'use strict';

  $( function () {
    $( 'body' ).html( '<h1>hello world!</h1>' );
  });

}));

[Victorystick]

@Rich-Harris Nice! Missed that one... 😝 Should the issue be closed?

Do you have somewhere to place documentation? Do you prefer the Wiki or some Markdown file?

[Victorystick]

What are your thoughts on the ES6 renaming issue of imported bindings? Should getCanonicalName be changed to be ES6 aware? Unfortunately it is used before generate is called, and only then is the final output format known.

Should we perhaps use some other form of identifier mapping technique?

[Rich-Harris]

@Victorystick we're definitely thinking along the same lines. It might be easier to wait until generate is called before deconflicting (wherein getCanonicalName is called) for this exact reason. Will have a quick crack at that and see what falls out

[Rich-Harris]

Have got all the tests passing. Something about it seems slightly hacky (particularly caching the results of getCanonicalName separately depending on whether the output is an ES6 module), but not catastrophically so (I hope). In any case, I'm thinking about a slightly wider-reaching refactor made possible by the fact that sorting now happens at the module (rather than statement) level – it means that modules could be responsible for rendering themselves, which (as well as being a good deal more memory-efficient, since each statement doesn't need its own MagicString instance) might have implications for how bundle-level canonical names are determined.

[Rich-Harris]

Do you have somewhere to place documentation? Do you prefer the Wiki or some Markdown file?

There's a bare-bones wiki, with some stuff cribbed from the Esperanto wiki, and a lot of 'coming soon...'

[Victorystick]

What do you think of storing all names referenced in a bundle in some central collection? It could handle deconfliction of names separately from the Bundle/Module code. Later (during generation) it could be queried on a per-module basis (including perhaps some format specific information) to get the name to use when generating the output code.

Something like (in the context of a Module):

this.nameStore.getCanonicalName( name: string, direct: boolean ) // -> string

The direct boolean could indicate the directness of the identifier access.
ES6 modules and SystemJS modules access variables directly, while CJS, AMD, UMD, and IIFE do not.

import { basename } from 'path';
basename('x/y.js');

// In CJS, AMD, UMD, and IIFE this becomes

path.basename('x/y.js');

// While in SystemJS it stays:

basename('x/y.js');

I think that would be nicer than passing es6 along, although it's a fair solution to the immediate problem.

[Rich-Harris]

Certainly feels easier to reason about, having a bundle-level collection. It wouldn't be flat though – in situations like this...

// foo.js
export default 'a';

// bar.js
import a from './foo';
export { a as b };

// baz.js
import { b as c } from './bar';

...either each module needs to keep track of which names it needs to change, or the bundle needs to do that on behalf of each module, so at the very least it would need to be this:

this.nameStore.getCanonicalName( module, moduleLocalName, direct )

Not really sure which is better (or if there's a third way).

[Victorystick]

I'll look into the possibilities of such an approach. 👍

[Rich-Harris]

Shall we merge #62 in the meantime, so we've at least got test coverage and a base to refactor from, or you do reckon it's better to drop that and pursue the alternative?

[Victorystick]

Yes, I think so. Merge it!

[Rich-Harris]

Great, have merged that into master - will close this