Conversation
Though, install should just look at the lockfile?
|
|
||
| pushDeps('dependencies', {hint: null, optional: false}, true); | ||
| pushDeps('devDependencies', {hint: 'dev', optional: false}, !this.config.production); | ||
| if (!this.config.production) { |
There was a problem hiding this comment.
This breaks 11 other tests... However, I question the validity of at least one of the breaking specs:
FAIL __tests__/commands/import.js
● import missing dev deps in production
There was a problem hiding this comment.
Yeah, I've seen this import test to fail some times I think
|
|
||
| test.concurrent('--production flag ignores dev dependencies', () => { | ||
| return runInstall({production: true}, 'install-production', async (config) => { | ||
| return runInstall({production: true}, 'install-production-without-dev', async (config) => { |
There was a problem hiding this comment.
I think it's better to add a new test rather than change existing one if the new one covers a different case
There was a problem hiding this comment.
Sounds good.
The first test was too basic. This new integration test covers a more common case with nested dependencies and dev dependencies.
There was a problem hiding this comment.
Basic tests are good :) If they break, we don't have to debug which part of the test is failing
|
@juanca, we have merged recently a new fix to hoisting algorithm. |
|
Will do! Thanks for the update. |
Conflicts: __tests__/commands/install/integration.js
|
Well, I seem to be breaking less tests with the updated code. I might just:
|
|
Also, when you have a minute of spare time, do you mind linking the hoisting fix PR? I'm interested in knowing more about yarn's hoisting mechanisms. |
|
This PR fixed dev dependency hoisting bug #2921 |
|
I got an intermittent failure. :/ Any way to rebuild from here? |
|
You mean |
|
The test passes, does it mean that your case now works? |
| @@ -0,0 +1,1219 @@ | |||
| # THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. | |||
There was a problem hiding this comment.
That is quite a big dependency graph, it would increase the test run time.
Is it possible to get cover the case with a smaller example?
There was a problem hiding this comment.
Sure. I'll try to come up with something else. Ensure that is fails pre-fix and ensure it is green post-fix.
|
Yeah, the fix made my case work! We will be testing in staging (then production) servers once the fix is released. I'll try to come up with a smaller / optimized use cases for tests. Most likely going to make extra packages in e2e test repository. |
|
Awesome, thanks for investing in stability |
|
@juanca thanks for the PR. |
ghost
mentioned this pull request
3 participants
Summary
I'm not sure if this is a substantial feature request. However, I believe the issue is severe and should be addressed immediately.
As of yarn v0.18.0, yarn will sometimes install development dependencies when using the production flag (
--production). This PR introduces a failing spec from an existing issue.Test plan
Test driven development: See Implement failing spec for accidentally hoisting dev dependencies
Halp
I would appreciate some guidance on the files to look at for possible fixes. I've dived a bit and it looks like hoisting might be the reason for installing dev dependencies: when resolving the dependency tree, dev dependencies are still considered -- which is not straightforward -- and used in the install process.
My initial thought is to avoid any dev dependencies and use the lockfile's resolved list of packages (not in dev dependencies) when invoking
yarn install --production-- and essentially avoid any type of hoisting.