ef4 / ember-browserify

ember-cli addon for easily loading CommonJS packages from npm via browserify.
MIT License
172 stars 28 forks source link

Cannot `import { foo } from 'npm:package' #40

Closed devinus closed 8 years ago

devinus commented 9 years ago

If an npm package exports { foo: function() {} } I cannot use the ES6 object shorthand to import the function. Only import package from 'npm:package' syntax works.

ef4 commented 9 years ago

Yeah, it would be nice to make this work. It's a little tricky because ES6 modules don't really have the same semantics as browserify modules. For example, in ES6:

import { thing } from 'some-module';

Is not equivalent to

import someModule from 'some-module';
var thing = someModule.thing;
dortort commented 9 years ago

any plans on making this work?

asakusuma commented 8 years ago

No plans at the moment for a couple reasons:

  1. Like Edward said, the concept of exporting members (as opposed to a single object) doesn't really exist in npm/browserify. We could still do it, but it's an "icing on the cake" kind of feature.
  2. While the syntactic sugar would be nice, not sure it's worth implementing when a much better solution to this problem is on the horizon: https://github.com/ember-cli/ember-cli/issues/4211
dfreeman commented 8 years ago

Given that the packager stuff has been "on the horizon" for a while now, would it be reasonable to revisit this?

I've seen an increasing number of npm packages that are written in ES6 and then transpiled via Babel before release. Such modules have an __esModule key defined on their exports to distinguish them from standard CommonJS ones.

Would you consider a PR that changes the behavior when that key is set, presumably tied to an ember-browserify configuration option to avoid breaking backwards compatibility?

asakusuma commented 8 years ago

@dfreeman not opposed. Can you elaborate how this could break backwards compatibility?

Just to make sure we're on the same page, what you're going to do is look for named imports, and if a named import exists AND __esModule is true, than make the assumption enumerated by @ef4 above?

dfreeman commented 8 years ago

I don't think we can use named imports as an opt-in, because transpiled modules may be in this format and also have default exports, e.g.

Object.defineProperty(exports, "__esModule", {
  value: true
});
var x = exports.x = 'foo';
exports.default = 'bar';

Today if you imported from that module, you'd get:

import myPackage from 'npm:my-package';
// myPackage is { x: 'foo', default: 'bar' }

I'm proposing adding a check similar to what Babel emits by default when targeting CommonJS:

return obj && obj.__esModule ? obj : { default: obj };

This would allow for proper interop with both named and default exports as you'd expect:

import myPackage from 'npm:my-package';
// myPackage is 'bar'

In the above case, you'd still want to honor the __esModule flag even though there's no named import occurring. It doesn't work in the browser, but you can see all the moving parts in the emitted code here.

My take is that supporting both named and default exports from modules like this is desirable change. However, it's also a breaking one for anyone working around the current behavior by manually accessing e.g. myPackage.default, which is why I'd propose making it configurable.

asakusuma commented 8 years ago

Ah, so I'm wondering if we can support this by remapping how import statements are transpired, instead of modifying how we define the imported modules.

dfreeman commented 8 years ago

It's entirely possible I'm missing something, but it seems as though the ambiguity around default imports/exports is still a problem whether you do the mapping on the import side or in the stub, isn't it?

asakusuma commented 8 years ago

In theory, we could modify the import code. Just realized this would be a little harder now since ember-cli 2.x passes ES5 code to postprocessTree instead of ES5 code. The general idea would be to leave the stub the same and then modify the import code if there's a deterministic way to know from ES5 code that someone is trying to do a named import.

For anyone else following along, here is code that compares the two different ways to import a named export: http://babeljs.io/repl/#?evaluate=true&presets=es2015%2Creact&experimental=true&loose=false&spec=false&playground=false&code=import%20%7B%20named%20%7D%20from%20'npm%3Abar'%3B%0A%0Anamed()%3B%0A%0Aimport%20baz%20from%20'npm%3Abaz'%3B%0A%0Abaz.named()%3B

dfreeman commented 8 years ago

Taking a step back, I think there are two different possible goals here:

The first bullet point was the original question in this issue, and I'm realizing now I think I ended up hijacking the thread because it was closely related to my desire to solve the second one. I agree with @ef4's original statement that trying to use named imports to access old school node exports is probably more trouble than it's worth, because they just don't line up one-to-one with with the semantics of ES6 modules.

In the time since the issue was originally filed, though, the trend of publishing transpiled modules to npm has grown, though, so what I'd love to see is a solution to the second bullet point using the same technique Babel itself uses in CommonJS environments.

If I'm using two node packages, one that's plain old ES5 and the other that was transpiled from ES6 before publishing:

// some ES5 node package
exports.hi = function() { /* ... */ };
exports.hello = function() { /* ... */ };
// some transpiled node package
Object.defineProperty(exports, '__esModule', { value: true });
exports.default = function FizzBuzzer() { /* ... */ };
exports.isFizz = function isFizz() { /* ... */ };
// ...

If I'm consuming these from another node package and using Babel myself, Babel will check for that __esModule flag and treat the first module as one with only a default export, and the second as an actual ES6 module with both default and named exports.

import greeter from 'greeter';
import FizzBuzzer, { isFizz } from 'fizz-buzzer';

greeter.hi(new FizzBuzzer());
greeter.hello(isFizz(5));

Babel wouldn't allow import { hi } from 'greeter'; in the same way ember-browserify doesn't today, because the export semantics are unclear.

I'm proposing aligning ember-browserify's behavior with what Babel currently does in node, so that "classic" node modules continue to be treated as only having a default export, but exports that were originally written as ES6 modules can be consumed in a parallel way to how they were written.

Sorry for the novel. I'm happy to talk in more detail on Slack if that's easier, or if this straight-up isn't something ember-browserify wants to do, that's cool too – I just wanted to make sure we're talking about the same thing :smile:

asakusuma commented 8 years ago

Why wouldn't babel allow import { hi } from 'greeter';? Perhaps I am missing something, but I didn't think babel was informed about the target module when it encounters an import statement. It just takes an import statement and assumes __esModule. So this ES6 code would work with your ES5 example module.

I am leaning more and more towards wanting to just modify the stubs for any transpiled ES6 modules and bumping a major version.

dfreeman commented 8 years ago

You're totally right – I was thinking the _interopRequireDefault shim would get in the way, but I missed that that's only invoked for default imports. Sorry for the noise!

I like the idea of a major version bump, since this is a breaking change but does unlock nicer behavior. It's a shame matching Babel's behavior is tricky; now I understand why you were talking about doing a rewrite on the import side.

dfreeman commented 8 years ago

Actually, making named imports "just work" for ES5 source modules may still be doable on the stub side alone.

return obj && obj.__esModule ? obj : Object.create(obj, { default: { value: obj } });

I'd need to actually play with this in live code to convince myself it works, but I think for ES5 modules that should give both the values set on exports as well as a default that points to the original exported object.

dfreeman commented 8 years ago

Scratch that. Object.create(function() {}) isn't callable, so that would break any ES5 module that does module.exports = function() { ... }. We could try and get cute with detecting the type of the export, but that may well be getting to the point of more trouble than it's worth.

asakusuma commented 8 years ago

Does Object.create need to be used?

dfreeman commented 8 years ago

You could do it without Object.create if you're willing to mutate the exports object itself. Something like this might work:

if (Object.isExtensible(obj)) {
  if (!obj.__esModule) {
    obj.default = obj;
  }
  return obj;
} else {
  return { default: obj };
}

(Worth noting both Object.create and Object.isExtensible both only work in IE >= 9)

stefanpenner commented 8 years ago

This seems out of scope, the CJS <-> es6 module boundary can be a-bit messy, and what is described is going form low fidelity to high fidelity and will have issues.

Let me instead recommend using destructing instead (which is what it seems like { } usage proposed here is trying to accomplish:

import RSVP from 'npm:rsvp';

const { hash } = RSVP;