Add Yarn option #105
Add Yarn option #105
Conversation
|
LGTM, thanks for pushing this @trentmwillis! Tests are failing however, yarn complains about some version mismatch. I have the suspicion that it behaves differently than npm, by checking the npm registry for our Maybe renaming |
| @@ -91,7 +91,8 @@ function installPristineApp(appName, options) { | |||
| hasNodeModules, | |||
| hasBowerComponents, | |||
| emberVersion: options.emberVersion || 'canary', | |||
| emberDataVersion: options.emberDataVersion || 'emberjs/data#master' | |||
| emberDataVersion: options.emberDataVersion || 'emberjs/data#master', | |||
| yarn: options.yarn || false | |||
There was a problem hiding this comment.
I am asking myself if it could make sense to not default to false, but instead check if the addon has a yarn.lock file, and default to true in this case? ember new needs the --yarn option to use yarn, because there is no app yet, but when running ember install it does a similar check afaik, assuming it should use yarn if the project uses it.
Maybe we could copy that behavior here? Fwiw, this could be a separate PR though...
There was a problem hiding this comment.
Is there a way to assume yarn usage like ember-cli does? I created ember-cli/rfcs#107 to possibly help.
| @@ -43,15 +43,16 @@ describe('Acceptance | application', function() { | |||
| promoteHtmlbars(); | |||
|
|
|||
| return app.create('dummy', { | |||
| fixturesPath: 'tests' | |||
| fixturesPath: 'tests', | |||
| yarn: true | |||
There was a problem hiding this comment.
perhaps we should leave this test intact and make a new one that tests yarn. Then both our npm and yarn code paths get tested.
|
|
||
| cache: | ||
| - '%APPDATA%\npm-cache' | ||
| - "%LOCALAPPDATA%\\Yarn" |
There was a problem hiding this comment.
accidental double slash?
| return runCommand('yarn'); | ||
| } else { | ||
| return runCommand('npm', 'install'); | ||
| } |
There was a problem hiding this comment.
Following up on my comment https://github.com/tomdale/ember-cli-addon-tests/pull/105/files#r118477645 and I think also what @kellyselden was suggesting, if this addon knows which package manager to use, maybe we can abstract this and the yarn add thing above away into a reusable method and make it public (like app.install() and app.install('my-addon') ) which users of this addon can use to customize their test setup without having to care about which package manager shall be used (e.g. here https://github.com/kaliber5/ember-fastboot-addon-tests/blob/master/lib/util/app-manager.js#L41)?
Can be a separate PR as well, just wanted to mention my thoughts here...
|
@trentmwillis Just wondering, are you able to continue work on this? If you don't have time, maybe someone other can take this over? Would love to see this land... Apart from the comments above and the failing test, there are now some merge conflicts that need to be addressed... (@kellyselden has been on fire lately 😉 ) |
|
Yeah, sorry, I likely won't get to this any time soon, so if someone else would like to pick it up, go for it! |
Fixes #76. Provides a nice speed boost.