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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

dfreeman
Copy link
Contributor

@dfreeman dfreeman commented Jun 2, 2016

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
Copy link

machty commented Apr 15, 2017

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
Copy link

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
Copy link

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
Copy link

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
Copy link
Contributor

Kerrick commented Jul 10, 2017

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
Copy link
Owner

ef4 commented Jul 11, 2017

Rebased and merged as bfc720e

@ef4 ef4 closed this Jul 11, 2017
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.

7 participants