Set bowerDirectory for project #2287

quaertym · 2014-10-10T19:38:44Z

I need this for this addon https://github.com/quaertym/ember-cli-dependency-checker, but might be useful in other places. Project can know its bowerDirectory, right?

stefanpenner · 2014-10-10T20:28:49Z

mind adding a quick test

quaertym · 2014-10-10T20:31:15Z

Sure.

quaertym · 2014-10-10T20:45:01Z

@stefanpenner done.

rwjblue · 2014-10-10T20:49:44Z

Not sure here. Seems like we should just put the logic to determine the default bowerDirectory into the project....

rwjblue · 2014-10-10T20:51:17Z

My concern is that if we only add it to the project in the broccoli build, many many other hooks do not have access to it.

I generally do not like having the same model (project) that has inconsistent properties.

quaertym · 2014-10-10T20:59:05Z

@rwjblue do you think that bowerDirectory belongs to the project rather than the app?

quaertym · 2014-10-12T12:35:27Z

@rwjblue I will close this PR and move bowerDirectory logic inside project, then set it for the app when in app's constructor, are you OK with that?

rwjblue · 2014-10-12T12:43:49Z

@quaertym - The changes sound good, but you do not need to close the PR. You can just amend your current commit and force push.

quaertym · 2014-10-12T15:41:40Z

@rwjblue @stefanpenner Can you guys review this?
Some points:

I had to set bowerDirectory for app in its constructor to ensure backwards compatibility.
I had to change MockProject in order to make tests pass. You can checkout the diff.
This test https://github.com/stefanpenner/ember-cli/blob/master/tests/acceptance/help-test.js#L25 fails unrelated to my changes on OS X. (edit: but not for travis, hmm)

rwjblue · 2014-10-12T16:25:19Z

lib/models/project.js

@@ -19,6 +19,13 @@ function Project(root, pkg) {
  this.pkg           = pkg;
  this.addonPackages = {};
  this.liveReloadFilterPatterns = [];
+
+  var bowerrcPath = path.join(this.root, '.bowerrc');


Can you move this into a function (perhaps setupBowerDirectory or something) that is called from here instead of inlining it?

rwjblue · 2014-10-12T16:26:39Z

@quaertym - Looking good, we will also need some unit tests for this.

quaertym · 2014-10-12T20:27:56Z

@rwjblue Tests are in. I think it is good to merge.

quaertym · 2014-10-13T12:13:50Z

@rwjblue @stefanpenner Can you merge this? Travis is happy, appveyor is not.

rwjblue · 2014-10-13T12:58:16Z

lib/models/project.js

@@ -14,11 +14,21 @@ var forOwn  = require('lodash-node/modern/objects/forOwn');

 var emberCLIVersion = require('../utilities/ember-cli-version');

+Project.prototype.setupBowerDirectory = function() {


While it is perfectly valid to put this method above the constructor, it is odd. Can you move it below?

Tests do not run when it is below.

rwjblue · 2014-10-13T13:00:26Z

I'd like to see a few more project unit tests for the following scenarios:

No .bowerrc
.bowerrc with invalid JSON
.bowerrc without a directory property

You can use fixtures to setup the project under test with a custom root with the needed files from above.

ahacking · 2014-10-14T23:23:48Z

Will the bower directory be made available within the Brocfile so you can app.import your bower components from whatever path has been configured?

It would also allow addons to do imports for you without making assumptions unless you already have this figured out.

quaertym · 2014-10-14T23:28:20Z

@ahacking You can still access it using app.bowerDirectory as before in Brocfile, see https://github.com/quaertym/ember-cli/blob/patch-13/lib/broccoli/ember-app.js#L76

ahacking · 2014-10-14T23:32:55Z

Sweet! I just read another issue with someone using an old version of ember CLI and they were importing using an assumed location so I wasn't sure if the bower directory was available or not.

This is good, I'm wondering if there would be value in say a bowerImport helper to save some string munging in our Brocfiles?

quaertym · 2014-10-14T23:38:22Z

Not in this PR, but I liked the idea. It can transform the code from this:

app.import(app.bowerDirectory + '/modernizr/modernizr.js');

to this:

app.bowerImport('/modernizr/modernizr.js');

quaertym · 2014-10-14T23:47:21Z

@rwjblue Can you review this PR one more time?

Set bowerDirectory for project

rwjblue · 2014-10-15T00:08:22Z

Beautiful!! Thanks @quaertym!

quaertym · 2014-10-15T00:16:05Z

👍

@rwjblue What is your opinion on bowerImport method suggested above by @ahacking? I can implement this.

rwjblue · 2014-10-15T00:37:42Z

I'd say probably not. I do not want more public API for importing things (app.import is crazy enough).

Rules seem simple (we can publish them on the website or something), but here you go:

in your Brocfile, use the path that the file exists on disk
in an addon, use app.bowerDirectory or project.bowerDirectory to ensure you are not tied to the specific location.

jakecraige mentioned this pull request Oct 10, 2014

Addon initialization confusion in codebase #2288

Closed

quaertym force-pushed the patch-13 branch from f1570c4 to 66ad3a3 Compare October 10, 2014 20:44

quaertym force-pushed the patch-13 branch from 66ad3a3 to 1f5d04e Compare October 12, 2014 15:33

rwjblue reviewed Oct 12, 2014
View reviewed changes

quaertym force-pushed the patch-13 branch 2 times, most recently from 4f8722f to a5798b7 Compare October 12, 2014 20:16

rwjblue reviewed Oct 13, 2014
View reviewed changes

quaertym force-pushed the patch-13 branch from a5798b7 to 6b06367 Compare October 14, 2014 23:39

quaertym force-pushed the patch-13 branch from 6b06367 to fe04933 Compare October 14, 2014 23:50

Move bowerDirectory inside project

3046af9

quaertym force-pushed the patch-13 branch from fe04933 to 3046af9 Compare October 15, 2014 00:01

rwjblue added a commit that referenced this pull request Oct 15, 2014

Merge pull request #2287 from quaertym/patch-13

cf5f8ed

Set bowerDirectory for project

rwjblue merged commit cf5f8ed into ember-cli:master Oct 15, 2014

jakecraige mentioned this pull request Jan 6, 2015

define is not defined #2890

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set bowerDirectory for project #2287

Set bowerDirectory for project #2287

quaertym commented Oct 10, 2014

stefanpenner commented Oct 10, 2014

quaertym commented Oct 10, 2014

quaertym commented Oct 10, 2014

rwjblue commented Oct 10, 2014

rwjblue commented Oct 10, 2014

quaertym commented Oct 10, 2014

quaertym commented Oct 12, 2014

rwjblue commented Oct 12, 2014

quaertym commented Oct 12, 2014

rwjblue Oct 12, 2014

rwjblue commented Oct 12, 2014

quaertym commented Oct 12, 2014

quaertym commented Oct 13, 2014

rwjblue Oct 13, 2014

quaertym Oct 14, 2014

rwjblue commented Oct 13, 2014

ahacking commented Oct 14, 2014

quaertym commented Oct 14, 2014

ahacking commented Oct 14, 2014

quaertym commented Oct 14, 2014

quaertym commented Oct 14, 2014

rwjblue commented Oct 15, 2014

quaertym commented Oct 15, 2014

rwjblue commented Oct 15, 2014

		@@ -14,11 +14,21 @@ var forOwn = require('lodash-node/modern/objects/forOwn');

		var emberCLIVersion = require('../utilities/ember-cli-version');

		Project.prototype.setupBowerDirectory = function() {

Set bowerDirectory for project #2287

Set bowerDirectory for project #2287

Conversation

quaertym commented Oct 10, 2014

stefanpenner commented Oct 10, 2014

quaertym commented Oct 10, 2014

quaertym commented Oct 10, 2014

rwjblue commented Oct 10, 2014

rwjblue commented Oct 10, 2014

quaertym commented Oct 10, 2014

quaertym commented Oct 12, 2014

rwjblue commented Oct 12, 2014

quaertym commented Oct 12, 2014

rwjblue Oct 12, 2014

Choose a reason for hiding this comment

rwjblue commented Oct 12, 2014

quaertym commented Oct 12, 2014

quaertym commented Oct 13, 2014

rwjblue Oct 13, 2014

Choose a reason for hiding this comment

quaertym Oct 14, 2014

Choose a reason for hiding this comment

rwjblue commented Oct 13, 2014

ahacking commented Oct 14, 2014

quaertym commented Oct 14, 2014

ahacking commented Oct 14, 2014

quaertym commented Oct 14, 2014

quaertym commented Oct 14, 2014

rwjblue commented Oct 15, 2014

quaertym commented Oct 15, 2014

rwjblue commented Oct 15, 2014