Implicitly add an npm dev dependency on bower #157
Conversation
| var adapter = this; | ||
| debug('Installing bower via npm'); | ||
| return adapter.run('npm', [].concat(['install', 'bower@^1.3.12']), { cwd: adapter.cwd }); | ||
| return findEmberPath(adapter.cwd).then(function(emberPath) { |
There was a problem hiding this comment.
I know you didn't add this, but I wonder if finding bower relative to ember-cli is necessary anymore?
There was a problem hiding this comment.
@kategengler hmm good point. So presumably the logic would be
- run npm before bower
- trust that bower exists in
PROJECT_ROOT/node_modules/.bin/bowerand assert otherwise
There was a problem hiding this comment.
Yeah, or we can just leave this since it works, but just thought it worth thinking about
There was a problem hiding this comment.
@kategengler leaving it in this PR seems like the right thing to do; it'll be easier to track & revert in a separate PR if it causes issues (although I think you're right that finding it relative to ember-cli would no longer be useful).
I was getting trolled by npm@5 in ember-try, which is fixed in ember-cli/ember-try#157 but there's no reason for us to use bower here anyway.
global bower and use if available. Error if neither local or global bower is available.
For scenarios that 1. do not already have a dependency or dev dependency on bower and 2. have bower dependencies Also assert that 1. npm is ran before bower 2. if bower is run, a local bower is found
rwjblue
force-pushed
the
hjdivad/install-bower-via-npm-dep-mgr
branch
from
2a56af3
to
ea8afb7
Compare
|
OK, one of the CI jobs (the
Seemed bizarre (since that job took only 12 minutes on the last merge), I restarted it... |
|
Ah, most of the scenarios in client-test were only using bower, but with this PR they will each be npm installing, which could account for the huge increase in time. Might be worth rethinking client-test; I think we can remove most of the deprecated commands, at the very least. |
|
OK, I removed deprecated commands in 9f10230 (please double check me 😄). Hoping this shortened things up enough... |
|
So did I in #160 Looks fine, I removed some more in my PR, but I can update it after this merges. |
|
Shoot, sorry. I didn’t check my GH notifications before updating... |
For scenarios that
Also assert that
This PR is a continuation of, and supersedes, #151.
Fixes #141
Fixes #150