Prevent running babel twice. #6530

rwjblue · 2016-12-05T20:55:04Z

We currently transpile the output of all addon's addon/ trees twice. Once inside the addon's treeForAddon step (this one excludes module transpilation by adding es6.modules to babel's blacklist) and the second time in lib/broccoli/ember-app.js.

This was originally done because we didn't require that addon's include their own preprocessors for JS (mostly due to performance reasons IIRC). In the time since that decision was made, we have swapped from a few different systems es6-module-transpiler -> esperanto -> babel. Now that our module transpilation step is using the same thing as we recommend addon's use (ember-cli-babel) what we are doing seems pretty whacky.

This PR removes the double processing of addon's addon/ files through babel, and includes a deprecation message for the scenario that originally prompted the second pass (where an addon didn't include a module transpiling preprocessor).

TODO:

Confirm that we have tests that already confirm this is working properly (I believe that our existing nested addon tests cover these changes, but I'd like to confirm).
Consider / discuss backporting the deprecation message for addons that do not have a transpiler and are relying on the global processing step.
Add helpful error if an addon overrides this.options.babel.compileModules
Implement logic to avoid deprecation being added in Move custom options to "ember-cli-babel" options hash emberjs/ember-cli-babel#105.
Split out findAddonByName into a utility method to be shared
r?

Addresses parts of #5015

rwjblue · 2016-12-05T21:58:49Z

lib/models/addon.js

+
+    this.options = defaultsDeep(this.options, {
+      babel: {
+        // TODO: coordinate with @turbo87 on exactly how to do


/cc @Turbo87

rwjblue · 2016-12-05T21:59:13Z

lib/models/addon.js

@@ -797,16 +831,23 @@ var Addon = CoreObject.extend({
  compileAddon: function(tree) {
    this._requireBuildPackages();

-    var reexported;
+    if (!this.options.babel.compileModules) {
+      throw new Error('y u clobber!?!!?!');


Remove this before landing, was using it as a debugging tool

rwjblue · 2016-12-05T21:59:32Z

lib/models/addon.js

+      if (!registryHasAddon(this.registry, 'ember-cli-babel')) {
+        this._warn('Addon files were detected in `' + this._treePathFor('addon') + '`, but `ember-cli-babel` ' +
+                              'is not present as a dependency in `' + this.name + '`. ' +
+                              'Please make sure to add `ember-cli-babel`) ' +


Weird ) mid-sentence here

This moves the modules transpilation step to be done completely by `lib/models/addon.js`, and does not leave additional steps left to be done by `lib/broccoli/ember-app.js`.

Template preprocessors emit ES modules, and once the two pass babel transpilation is removed we need to ensure templates get ES -> AMD processing.

Prior to the changes in this PR, all addon/ files were processed by babel twice (once by the addon itself excluding modules, then again by ember-cli including modules). This meant it was possible to "forget" to include an ember-cli-babel dep when you really needed one (because you have JS files in `addon/`).

Newer versions of ember-cli-babel will begin deprecating passing any options in `babel` config hash that are not destined for `babel` itself (this includes `compileModules` and `includePolyfill`). This detects the newer version and uses the appropriate (non-deprecated) configuration property. It also moves `findAddonByName` into a shared utility method (to ensure both project and addon use the same mechanism).

stefanpenner · 2016-12-06T05:49:49Z

lib/broccoli/ember-app.js

    srcDir: 'modules',
    allowEmpty: true,
    annotation: 'Funnel: Addon JS'
  });

-  var transpiledAddonTree = new Babel(addonES6, this._prunedBabelOptions());


stefanpenner · 2016-12-06T05:49:53Z

lib/broccoli/ember-app.js

-  var transpiledAddonTree = new Babel(addonES6, this._prunedBabelOptions());
-  var trees = [ transpiledAddonTree ];
-
-  var reexportsAndTranspiledAddonTree = mergeTrees(trees, {


stefanpenner · 2016-12-06T05:53:55Z

lib/models/addon.js

+        this._warn('Addon files were detected in `' + this._treePathFor('addon') + '`, but no JavaScript ' +
+                   'preprocessors were found for `' + this.name + '`. Please make sure to add a preprocessor ' +
+                   '(most likely `ember-cli-babel`) to in `dependencies` (NOT `devDependencies`) in ' +
+                   '`' + this.name + '`\'s `package.json`.');


as it may be possible to have more then 1 addon by this name, should we also print its path on disk?

The full path absolute path to the addon's addon/ directory is printed in this message, so I don't think changes are needed in this one for that purpose...

stefanpenner · 2016-12-06T05:56:00Z

lib/models/addon.js

+  return new Babel(tree, {
+    modules: 'amdStrict',
+    moduleIds: true,
+    whitelist: ['es6.modules']


do we need to provide the amd-name-resolver? As this doesn't seem to get the config from 146

Confirm, will update

stefanpenner · 2016-12-06T05:58:03Z

lib/models/addon.js

+    // if used in the older way
+    //
+    // see: https://github.com/babel/ember-cli-babel/pull/105
+    var emberCLIBabelInstance = findAddonByName(this.addons, 'ember-cli-babel');


might be reason to add this.findOwnAddonByName('ember-cli-babel') as an API.

Agreed, but I wanted to decouple new public API's from this PR.

Technically, project.findAddonByName is also private (per YUIDocs IIRC), I'd suggest a future RFC to make that method public on both Project and Addon.

sounds very reasonable.

stefanpenner · 2016-12-06T05:59:51Z

lib/models/addon.js

@@ -730,7 +777,7 @@ var Addon = CoreObject.extend({
    if (addonTemplates) {
      standardTemplates = new Funnel(addonTemplates, {
        srcDir: '/',
-        destDir: 'modules/' + this.name + '/templates'
+        destDir: this.name + '/templates'


@nathanhammond mentioned some add-ons accidentally depend on this, we should likely see if any very popular ones absolutely do. And inform said authors to address.

@stefanpenner - The modules/ funneling is done a tad bit later (at the end of compileAddon method). In this case, we need the paths on disk to match what we expect the named AMD moduleIds will be.

This is done around L865 in this file.

I think both ember-a11y and liquid-fire depend on this

stefanpenner · 2016-12-06T06:01:02Z

lib/models/addon.js

+    if (!this.options[emberCLIBabelConfigKey].compileModules) {
+      throw new SilentError(
+        'Ember CLI addons manage their own module transpilation during the `treeForAddon` processing. ' +
+          '`' + this.name + '` has overridden the `this.options.' + emberCLIBabelConfigKey + '.compileModules` ' +


as there may be more then one addon by this name in node_modules, should we print out its whole path?

Sounds good, will do

* Use amd-name-resolver for modules only transpilation. * Share babel config between modules only transpilation and default babel config. * Add path to addon in error message (in case multiple share the same addon name).

rwjblue · 2016-12-06T06:15:35Z

Updated from initial round of review...

twokul · 2016-12-06T15:52:16Z

lib/models/addon.js

+}
+
+function registryHasPreprocessor(registry, type) {
+  return !!registry.load(type).length;


will return registry.load(type).length > 0 be clearer and easier to parse?

Yep, good idea!

twokul · 2016-12-06T15:55:35Z

lib/models/addon.js

@@ -730,7 +777,7 @@ var Addon = CoreObject.extend({
    if (addonTemplates) {
      standardTemplates = new Funnel(addonTemplates, {
        srcDir: '/',
-        destDir: 'modules/' + this.name + '/templates'
+        destDir: this.name + '/templates'


I think both ember-a11y and liquid-fire depend on this

rwjblue · 2016-12-06T16:09:47Z

@twokul - RE:

I think both ember-a11y and liquid-fire depend on this

Yes, modules/ shifting isn't removed here, just moved around (now its done at the very end of compileAddon (called from treeForAddon). There should be no semantic change here...

rwjblue · 2016-12-07T17:08:31Z

@homu r+

homu · 2016-12-07T17:08:32Z

📌 Commit f2e7c3e has been approved by rwjblue

homu · 2016-12-07T17:08:44Z

⚡ Test exempted - status

Prevent running babel twice. We currently transpile the output of all addon's `addon/` trees twice. Once inside the addon's `treeForAddon` step (this one excludes module transpilation by adding `es6.modules` to babel's blacklist) and the second time in [lib/broccoli/ember-app.js](https://github.com/ember-cli/ember-cli/blob/2df251ce018fd12570bdce72c167ff14a427fa74/lib/broccoli/ember-app.js#L929). This was originally done because we didn't require that addon's include their own preprocessors for JS (mostly due to performance reasons IIRC). In the time since that decision was made, we have swapped from a few different systems `es6-module-transpiler` -> `esperanto` -> `babel`. Now that our module transpilation step is using the same thing as we recommend addon's use (`ember-cli-babel`) what we are doing seems pretty whacky. This PR removes the double processing of addon's `addon/` files through babel, and includes a deprecation message for the scenario that originally prompted the second pass (where an addon didn't include a module transpiling preprocessor). TODO: - [x] Confirm that we have tests that already confirm this is working properly (I believe that our existing nested addon tests cover these changes, but I'd like to confirm). - [ ] Consider / discuss backporting the deprecation message for addons that do not have a transpiler and are relying on the global processing step. - [X] Add helpful error if an addon overrides `this.options.babel.compileModules` - [X] Implement logic to avoid deprecation being added in emberjs/ember-cli-babel#105. - [X] Split out `findAddonByName` into a utility method to be shared - [ ] r? Addresses parts of #5015

…jblue Fixups for recent two pass babel changes. We started testing out master in a very large app at work, and ran into a few small issues with the changes in #6530. This PR makes the following changes: * Makes the warnings much more useful (and prevents an error that was occurring just when reading config to determine if we should warn). * Cherry pick change from #6480, as it is now manifesting (due to internal changes in `modules/` shimming, the `overwrite` that was happening at a later stage, is now happening in `compileAddon`). * Ensure a full pass of babel transpilation is done in the fallback case (when an addon with `addon/**/*.js` files has no JS preprocessors). This is required due to the way the migration from esperanto to babel occurred where the second pass (that was intended for only modules transpilation) was actually implemented as a full babel transpile step.

mfazekas · 2017-02-24T13:47:23Z

@rwjblue the issue with ember-lodash seems to be the condition added in the PR:

if (this._fileSystemInfo().hasJSFiles) {
      var processedJsFiles = this.preprocessJs(this.addonJsFiles(tree), '/', this.name, {
       ...
}

ember-lodash itself has no JSFiles, but it's generating it's tree from .js files in other modules -
see
https://github.com/mike-north/ember-lodash/blob/master/index.js#L15

rwjblue commented Dec 5, 2016

View reviewed changes

rwjblue force-pushed the avoid-two-pass-babel branch 3 times, most recently from 9098cc5 to 69f4b5d Compare December 6, 2016 04:15

rwjblue added 3 commits December 5, 2016 23:20

Prevent two-pass babel transpilation.

b1125b8

This moves the modules transpilation step to be done completely by `lib/models/addon.js`, and does not leave additional steps left to be done by `lib/broccoli/ember-app.js`.

Ensure template output receives babel module transpilation.

ab68c30

Template preprocessors emit ES modules, and once the two pass babel transpilation is removed we need to ensure templates get ES -> AMD processing.

rwjblue force-pushed the avoid-two-pass-babel branch from 69f4b5d to fc8bf2b Compare December 6, 2016 04:20

rwjblue added the Needs Code Review label Dec 6, 2016

rwjblue force-pushed the avoid-two-pass-babel branch from fc8bf2b to 8281260 Compare December 6, 2016 05:13

rwjblue force-pushed the avoid-two-pass-babel branch from 8281260 to bacb837 Compare December 6, 2016 05:14

stefanpenner reviewed Dec 6, 2016

View reviewed changes

stefanpenner requested changes Dec 6, 2016

View reviewed changes

Cleanup per Stef's review comments.

6cd6fe7

* Use amd-name-resolver for modules only transpilation. * Share babel config between modules only transpilation and default babel config. * Add path to addon in error message (in case multiple share the same addon name).

twokul reviewed Dec 6, 2016

View reviewed changes

twokul mentioned this pull request Dec 6, 2016

Ensure trees are processed properly ember-a11y/ember-a11y#50

Merged

Address Alex's PR comment.

f2e7c3e

rwjblue force-pushed the avoid-two-pass-babel branch from d9bb736 to f2e7c3e Compare December 7, 2016 02:06

stefanpenner approved these changes Dec 7, 2016

View reviewed changes

Turbo87 added the bug label Dec 7, 2016

homu merged commit f2e7c3e into master Dec 7, 2016

Turbo87 deleted the avoid-two-pass-babel branch December 7, 2016 17:08

This was referenced Dec 7, 2016

All JS templates to be used in addons #6480

Closed

Properly call preprocessTree / postprocessTree for addons. #6516

Merged

rwjblue mentioned this pull request Dec 7, 2016

Fixups for recent two pass babel changes. #6538

Merged

This was referenced Jan 16, 2017

Issues with lazy engines importing modules in addon/routes.js. ember-engines/ember-engines#318

Closed

Not doing double-transpile makes writing tooling unhappy. #6687

Closed

mfazekas mentioned this pull request Feb 23, 2017

Could not find module lodash/merge in ember-cli 2.12 mike-north/ember-lodash#89

Closed

mfazekas mentioned this pull request Feb 24, 2017

Workaround for ember 2.12 beta issue mike-north/ember-lodash#93

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent running babel twice. #6530

Prevent running babel twice. #6530

rwjblue commented Dec 5, 2016 •

edited by stefanpenner

rwjblue Dec 5, 2016

rwjblue Dec 5, 2016

rwjblue Dec 5, 2016

stefanpenner Dec 6, 2016

stefanpenner Dec 6, 2016

stefanpenner Dec 6, 2016

rwjblue Dec 6, 2016

stefanpenner Dec 6, 2016

stefanpenner Dec 6, 2016

rwjblue Dec 6, 2016

stefanpenner Dec 6, 2016

rwjblue Dec 6, 2016

stefanpenner Dec 6, 2016

stefanpenner Dec 6, 2016

rwjblue Dec 6, 2016 •

edited

rwjblue Dec 6, 2016

twokul Dec 6, 2016

stefanpenner Dec 6, 2016

rwjblue Dec 6, 2016

rwjblue commented Dec 6, 2016

twokul Dec 6, 2016

rwjblue Dec 6, 2016

rwjblue Dec 6, 2016

twokul Dec 6, 2016

rwjblue commented Dec 6, 2016

rwjblue commented Dec 7, 2016

homu commented Dec 7, 2016

homu commented Dec 7, 2016

mfazekas commented Feb 24, 2017

Prevent running babel twice. #6530

Prevent running babel twice. #6530

Conversation

rwjblue commented Dec 5, 2016 • edited by stefanpenner

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwjblue Dec 6, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwjblue commented Dec 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwjblue commented Dec 6, 2016

rwjblue commented Dec 7, 2016

homu commented Dec 7, 2016

homu commented Dec 7, 2016

mfazekas commented Feb 24, 2017

rwjblue commented Dec 5, 2016 •

edited by stefanpenner

rwjblue Dec 6, 2016 •

edited