Skip to content
This repository has been archived by the owner on Mar 29, 2018. It is now read-only.

Add tests and basic support for batch exports. #177

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

eventualbuddha
Copy link
Contributor

cc @caridy @dominic

This is enough to make batch exports work for the bundle format, but not for CommonJS. Are there any tests that I should add that aren't covered by the pretty simple case I have here?

@caridy
Copy link
Contributor

caridy commented Oct 24, 2014

yeah, that should be it for now. maybe adding an extra export export var z = 8; in second.js.

I can't find any reference in the specs about overruling c, while under normal conditions you can't re-export the same identifier. Although it makes a lot of sense, I wonder how is that going to work. Imagine the scenario where first.js changes the value of c, and second.js also changes the value of c, for a format like System.register() this is going to be a problem since both values will be pushed into third.js.

/cc @guybedford @ericf

@caridy
Copy link
Contributor

caridy commented Oct 24, 2014

Also, I wonder why is this not enough for commonjs format, or any other format?

@eventualbuddha
Copy link
Contributor Author

@caridy commonjs seems to need more explicit work to support this since we may have to bake the indirection provided by batch exports into the output. That is, with the bundle format we know exactly where the terminal export is and what its identifier will be in the final output. For commonjs I believe we may have to go back to using getters to support this, i.e.

// a.js
export var a = 1;

// b.js
export * from './a';

// c.js
import { a } from './b';
console.log(a);

would output to this:

// a.js
var a = 1;
exports.a = a;

// b.js
var $$a = require('./a');

Object.defineProperties(exports, {
  a: {
    get: function() {
      return $$a.a;
    }
  }
});

// c.js
var $$b = require('./b');
console.log($$b.a);

An alternative approach would involve trying to reference the terminal export directly, which would get rid of the Object.defineProperties call. We may have to do some path manipulation to make it work, but it might be okay:

// a.js
var a = 1;
exports.a = 1;

// b.js
// in this scenario this becomes a no-op

// c.js
var $$a = require('./a');
console.log($$a.a);

Thoughts?

@guybedford
Copy link
Contributor

@caridy looking at the spec it seems like export * names that override existing local export names get ignored. This is from line 15.2.1.17 8. a) c) i).

Would these changes support an output format like System.registerDynamic?

@caridy
Copy link
Contributor

caridy commented Oct 24, 2014

@guybedford thanks for lookup this in the specs.

@eventualbuddha I think an alternative option is to modify the internals of export. Essentially, export * from './a'; will become export {a} from './a';, and doing so before the formatter kicks in since we can have all the named export and default export specifiers before processing the export *. What do you think?

@caridy
Copy link
Contributor

caridy commented Oct 24, 2014

After some discussion in IRC, it seems we will explorer the solution of computing the "terminal export" and make the export * noop.

@caridy
Copy link
Contributor

caridy commented Nov 14, 2014

@eventualbuddha any update on this one?

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.

3 participants