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

Prevent running babel twice. #6530

Merged
merged 6 commits into from Dec 7, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 2 additions & 9 deletions lib/broccoli/ember-app.js
Expand Up @@ -920,19 +920,12 @@ EmberApp.prototype._addonTree = function _addonTree() {
annotation: 'TreeMerger (addons)'
});

var addonES6 = new Funnel(addonTrees, {
var addonTranspiledModules = new Funnel(addonTrees, {
srcDir: 'modules',
allowEmpty: true,
annotation: 'Funnel: Addon JS'
});

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

var reexportsAndTranspiledAddonTree = mergeTrees(trees, {
annotation: 'TreeMerger: (re-exports)'
});

return this._cachedAddonTree = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

this._concatFiles(addonTrees, {
inputFiles: ['**/*.css'],
Expand All @@ -941,7 +934,7 @@ EmberApp.prototype._addonTree = function _addonTree() {
annotation: 'Concat: Addon CSS'
}),

this._concatFiles(reexportsAndTranspiledAddonTree, {
this._concatFiles(addonTranspiledModules, {
inputFiles: ['**/*.js'],
outputFile: '/addons.js',
allowNone: true,
Expand Down
105 changes: 89 additions & 16 deletions lib/models/addon.js
Expand Up @@ -26,6 +26,15 @@ var mergeTrees = require('../broccoli/merge-trees');
var Funnel = require('broccoli-funnel');
var walkSync = require('walk-sync');
var ensurePosixPath = require('ensure-posix-path');
var defaultsDeep = require('ember-cli-lodash-subset').defaultsDeep;
var Babel = require('broccoli-babel-transpiler');
var findAddonByName = require('../utilities/find-addon-by-name');

var DEFAULT_BABEL_CONFIG = {
modules: 'amdStrict',
moduleIds: true,
resolveModuleSource: require('amd-name-resolver').moduleResolve
};

function warn(message) {
if (this.ui) {
Expand All @@ -47,6 +56,17 @@ function deprecatedAddonFilters(addon, name, insteadUse, fn) {
};
}

function processModulesOnly(tree) {
var options = defaultsDeep({}, DEFAULT_BABEL_CONFIG);
options.whitelist = ['es6.modules'];

return new Babel(tree, options);
}

function registryHasPreprocessor(registry, type) {
return registry.load(type).length > 0;
}

/**
Root class for an Addon. If your addon module exports an Object this
will be extended from this base class. If you export a constructor (function),
Expand Down Expand Up @@ -126,6 +146,15 @@ var Addon = CoreObject.extend({
if (!this.name) {
throw new SilentError('An addon must define a `name` property.');
}

this.options = defaultsDeep(this.options, {
babel: DEFAULT_BABEL_CONFIG
});

var emberCLIBabelConfigKey = this._emberCLIBabelConfigKey();
this.options[emberCLIBabelConfigKey] = defaultsDeep(this.options[emberCLIBabelConfigKey], {
compileModules: true
});
},

hintingEnabled: function() {
Expand Down Expand Up @@ -307,6 +336,18 @@ var Addon = CoreObject.extend({
*/
_warn: warn,

_emberCLIBabelConfigKey: function() {
// future versions of ember-cli-babel will be moving the location for its
// own configuration options out of `babel` and will be issuing a deprecation
// if used in the older way
//
// see: https://github.com/babel/ember-cli-babel/pull/105
var emberCLIBabelInstance = findAddonByName(this.addons, 'ember-cli-babel');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds very reasonable.

var emberCLIBabelConfigKey = (emberCLIBabelInstance && emberCLIBabelInstance.configKey) || 'babel';

return emberCLIBabelConfigKey;
},

/**
Returns a given type of tree (if present), merged with the
application tree. For each of the trees available using this
Expand Down Expand Up @@ -637,7 +678,7 @@ var Addon = CoreObject.extend({
@return {Boolean} indicates if templates need to be compiled for this addon
*/
shouldCompileTemplates: function() {
return this._detectTemplateInfo().hasTemplates;
return this._fileSystemInfo().hasTemplates;
},

/**
Expand All @@ -651,14 +692,15 @@ var Addon = CoreObject.extend({
@return {Boolean} indicates if pod based templates need to be compiled for this addon
*/
_shouldCompilePodTemplates: function() {
return this._detectTemplateInfo().hasPodTemplates;
return this._fileSystemInfo().hasPodTemplates;
},

_detectTemplateInfo: function() {
_fileSystemInfo: function() {
if (this._cachedTemplateInfo) {
return this._cachedTemplateInfo;
}

var jsExtensions = this.registry.extensionsForType('js');
var templateExtensions = this.registry.extensionsForType('template');
var addonTreePath = this._treePathFor('addon');
var addonTemplatesTreePath = this._treePathFor('addon-templates');
Expand All @@ -677,6 +719,11 @@ var Addon = CoreObject.extend({
return file.match(podTemplateMatcher);
});

var jsMatcher = new RegExp('(' + jsExtensions.join('|') + ')$');
var hasJSFiles = files.some(function(file) {
return file.match(jsMatcher);
});

if (!addonTemplatesTreeInAddonTree) {
files = files.concat(this._getAddonTemplatesTreeFiles());
}
Expand All @@ -689,6 +736,7 @@ var Addon = CoreObject.extend({
});

this._cachedTemplateInfo = {
hasJSFiles: hasJSFiles,
hasTemplates: hasTemplates,
hasPodTemplates: hasPodTemplates
};
Expand Down Expand Up @@ -730,7 +778,7 @@ var Addon = CoreObject.extend({
if (addonTemplates) {
standardTemplates = new Funnel(addonTemplates, {
srcDir: '/',
destDir: 'modules/' + this.name + '/templates'
destDir: this.name + '/templates'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

@rwjblue rwjblue Dec 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done around L865 in this file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});

trees.push(standardTemplates);
Expand All @@ -743,7 +791,7 @@ var Addon = CoreObject.extend({

var podTemplates = new Funnel(tree, {
include: includePatterns,
destDir: 'modules/' + this.name + '/',
destDir: this.name + '/',
annotation: 'Funnel: Addon Pod Templates'
});

Expand All @@ -769,20 +817,20 @@ var Addon = CoreObject.extend({
this._requireBuildPackages();

if (this.shouldCompileTemplates()) {
var plugins = this.registry.load('template');

if (plugins.length === 0) {
if (!registryHasPreprocessor(this.registry, 'template')) {
throw new SilentError('Addon templates were detected, but there ' +
'are no template compilers registered for `' + this.name + '`. ' +
'Please make sure your template precompiler (commonly `ember-cli-htmlbars`) ' +
'is listed in `dependencies` (NOT `devDependencies`) in ' +
'`' + this.name + '`\'s `package.json`.');
}

return preprocessTemplates(this._addonTemplateFiles(tree), {
var processedTemplateTree = preprocessTemplates(this._addonTemplateFiles(tree), {
annotation: 'compileTemplates(' + this.name + ')',
registry: this.registry
});

return processModulesOnly(processedTemplateTree);
}
},

Expand All @@ -797,16 +845,28 @@ var Addon = CoreObject.extend({
compileAddon: function(tree) {
this._requireBuildPackages();

var reexported;
var emberCLIBabelConfigKey = this._emberCLIBabelConfigKey();
if (!this.options[emberCLIBabelConfigKey].compileModules) {
throw new SilentError(
'Ember CLI addons manage their own module transpilation during the `treeForAddon` processing. ' +
'`' + this.name + '` (found at `' + this.root + '`) has overridden the `this.options.' + emberCLIBabelConfigKey + '.compileModules` ' +
'value which conflicts with the addons ability to transpile its `addon/` files properly.'
);
}

var addonJs = this.processedAddonJsFiles(tree);
var templatesTree = this.compileTemplates(tree);

var trees = [addonJs, templatesTree, reexported].filter(Boolean);
var trees = [addonJs, templatesTree].filter(Boolean);

return mergeTrees(trees, {
var combinedJSAndTemplates = mergeTrees(trees, {
annotation: 'Addon#compileAddon(' + this.name + ') '
});

return new Funnel(combinedJSAndTemplates, {
destDir: 'modules/',
annotation: 'Funnel: compileAddon'
});
},

/**
Expand Down Expand Up @@ -852,7 +912,7 @@ var Addon = CoreObject.extend({
this._requireBuildPackages();

return new Funnel(tree, {
destDir: 'modules/' + this.moduleName(),
destDir: this.moduleName(),
description: 'Funnel: Addon JS'
});
},
Expand All @@ -878,9 +938,22 @@ var Addon = CoreObject.extend({
@return {Tree} Processed javascript file tree
*/
processedAddonJsFiles: function(tree) {
return this.preprocessJs(this.addonJsFiles(tree), '/', this.name, {
registry: this.registry
});
if (this._fileSystemInfo().hasJSFiles) {
var processedJsFiles = this.preprocessJs(this.addonJsFiles(tree), '/', this.name, {
registry: this.registry
});

if (!registryHasPreprocessor(this.registry, '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`.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


processedJsFiles = processModulesOnly(processedJsFiles);
}

return processedJsFiles;
}
},

/**
Expand Down
9 changes: 2 additions & 7 deletions lib/models/project.js
Expand Up @@ -14,6 +14,7 @@ var logger = require('heimdalljs-logger')('ember-cli:project');
var nodeModulesPath = require('node-modules-path');
var versionUtils = require('../utilities/version-utils');
var emberCLIVersion = versionUtils.emberCLIVersion;
var findAddonByName = require('../utilities/find-addon-by-name');

/**
The Project model is tied to your package.json. It is instantiated
Expand Down Expand Up @@ -524,13 +525,7 @@ Project.prototype.reloadAddons = function() {
Project.prototype.findAddonByName = function(name) {
this.initializeAddons();

function matchAddon(name, addon) {
return name === addon.name || (addon.pkg && name === addon.pkg.name);
}

return _.find(this.addons, function(addon) {
return matchAddon(name, addon);
});
return findAddonByName(this.addons, name);
};

/**
Expand Down
13 changes: 13 additions & 0 deletions lib/utilities/find-addon-by-name.js
@@ -0,0 +1,13 @@
'use strict';

var find = require('ember-cli-lodash-subset').find;

module.exports = function findAddonByName(addons, name) {
function matchAddon(name, addon) {
return name === addon.name || (addon.pkg && name === addon.pkg.name);
}

return find(addons, function(addon) {
return matchAddon(name, addon);
});
};
35 changes: 28 additions & 7 deletions tests/unit/models/addon-test.js
Expand Up @@ -622,7 +622,7 @@ describe('models/addon.js', function() {
});
});

describe('_detectTemplateInfo', function() {
describe('_fileSystemInfo', function() {
beforeEach(function() {
projectPath = path.resolve(fixturePath, 'simple');
var packageContents = require(path.join(projectPath, 'package.json'));
Expand All @@ -641,7 +641,7 @@ describe('models/addon.js', function() {
return [];
};

addon._detectTemplateInfo();
addon._fileSystemInfo();

expect(wasCalled).to.not.be.ok;
});
Expand All @@ -654,7 +654,7 @@ describe('models/addon.js', function() {
return [];
};

addon._detectTemplateInfo();
addon._fileSystemInfo();

expect(wasCalled).to.be.ok;
});
Expand All @@ -668,7 +668,8 @@ describe('models/addon.js', function() {
];
};

expect(addon._detectTemplateInfo()).to.deep.equal({
expect(addon._fileSystemInfo()).to.deep.equal({
hasJSFiles: true,
hasTemplates: true,
hasPodTemplates: true
});
Expand All @@ -683,7 +684,8 @@ describe('models/addon.js', function() {
];
};

expect(addon._detectTemplateInfo()).to.deep.equal({
expect(addon._fileSystemInfo()).to.deep.equal({
hasJSFiles: false,
hasTemplates: true,
hasPodTemplates: false
});
Expand All @@ -699,7 +701,8 @@ describe('models/addon.js', function() {
];
};

expect(addon._detectTemplateInfo()).to.deep.equal({
expect(addon._fileSystemInfo()).to.deep.equal({
hasJSFiles: false,
hasTemplates: true,
hasPodTemplates: false
});
Expand All @@ -715,7 +718,25 @@ describe('models/addon.js', function() {
];
};

expect(addon._detectTemplateInfo()).to.deep.equal({
expect(addon._fileSystemInfo()).to.deep.equal({
hasJSFiles: true,
hasTemplates: false,
hasPodTemplates: false
});
});

it('does not hasJSFiles when none found', function() {
addon._getAddonTreeFiles = function() {
return [
'components/',
'templates/',
'templates/components/',
'styles/foo.css'
];
};

expect(addon._fileSystemInfo()).to.deep.equal({
hasJSFiles: false,
hasTemplates: false,
hasPodTemplates: false
});
Expand Down