ef4 / ember-browserify

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

Handle ES6 syntax when inspecting AMD modules for npm imports #85

Closed dfreeman closed 7 years ago

dfreeman commented 8 years ago

I'm working on toy project where browser compatibility isn't a huge concern, so I've been playing with turning off most Babel transforms other than modules and experimental post-ES2015 stuff. This means debugging code in the browser looks more like what I actually wrote, and also has a positive impact on build times.

However, I found that doing this caused ember-browserify to stop finding my npm imports. After a little investigation, it turns out this is because the addon assumes any module that doesn't parse as valid ES5 must be using the modern module syntax.

Sadly, since ES2015 module loading is one of the lingering features that hasn't achieved wide rollout yet, it's totally possible for code like this to trickle through the Broccoli pipeline:

define('my-app/utils/some-module', ['exports', 'npm:some-package'], function(exports, _npmSomePackage) {
  exports.default = `some template string: ${ _npmSomePackage.default.MAGIC_VALUE }`;
});

It won't parse as valid ES5, but it also doesn't have any import statements in it, so the dependency on the some-package npm package is lost.

The proposed change here abandons the first pass attempting to parse modules as ES5, and instead does a single traversal recording both import statements and define() calls. In most scenarios you'd only ever see one or the other, but for consistency with today's behavior, if any ES6-style imports are found, those win out over anything that looks like an AMD define.

machty commented 7 years ago

I ran into a similar problem when debugging a tricky ember-concurrency bug; I disabled regenerator and suddenly this browserify import stopped working. I tried the code change in this PR and it seemed to fix the problem.

RobIsHere commented 7 years ago

I tried this, too and it didn't fix it for me. Neither with rebasing on 1.13 nor some other things I tried. I'll rather turn on babel again :(

thec0keman commented 7 years ago

Confirmed today this does work with ember-cli 2.13 / Babel 6 and turning off most transpiling. As targets become better utilized this fix will become more important. Is it possible to get this merged?

bendemboski commented 7 years ago

I also ran into this issue using targets to target only modern browsers, and running against the code in this PR fixed it, so I'd also love to see it merged!

Kerrick commented 7 years ago

Another confirmation here. We're using Ember CLI 2.14 and Babel 6 with targets set up to make debugging in development easier, so in development we also get these kinds of errors.

ef4 commented 7 years ago

Rebased and merged as bfc720e857dd8d03f91df45bf1f99aef12baf74d