Fix install --production not installing used packages, fixes #2468, #2263 #2537
Conversation
|
@bestander could you please take a look? I know you have a queue of PRs with different priority, but this one is 2 days as ready 😄 |
|
Oh lovely, will have a look today |
| @@ -793,7 +793,7 @@ test.concurrent('a subdependency of an optional dependency that fails should be | |||
| }); | |||
|
|
|||
| // disabled while fix is not merged | |||
There was a problem hiding this comment.
This comment should be removed
There was a problem hiding this comment.
Ah, missed that one
|
|
||
| _propagateNonIgnored() { | ||
| // | ||
| const tovisit: Array<HoistManifest> = []; |
There was a problem hiding this comment.
toVisit - according to code style in other places
| // | ||
| const depinfo = this._lookupDependency(info, depPattern); | ||
|
|
||
| // |
There was a problem hiding this comment.
spaces and // serve something?
There was a problem hiding this comment.
Keeping the code density in line with the rest of the code, nothing more. No problem to remove them.
|
@blexrob, great job! |
|
Oh, awesome! I have 2 projects at work that failed earlier, I'll give this a whirl in a couple of hours |
|
Works fine in my cases, great job! Doesn't fail anymore |
|
master now works with all 3 of my earlier failing test cases! wooo 🎉 |
|
I was planning to release 0.20 on Monday next week. |
|
If you want to send a PR to cherry-pick this into 0.19-stable, I'll merge it. |
|
@bestander Thanks! I have just tested #1808, and the failure there seems to be gone, too. |
…2468, yarnpkg#2263 (yarnpkg#2537) * Explicitly mark ignored deps of non-ignored packages as non-ignored (yarnpkg#761, yarnpkg#2468, yarnpkg#2263) * Fix style nits
They've fixed the issue (finally!) Ref yarnpkg/yarn#2537
|
Upgrading to 0.21.3 fixed my issue for for-in (for-own) |
Summary
When installing only production packages using
yarn install --production, the packagehoister sometimes ignores packages that are actually used. This can happen when an ignored package is deduped with another ignored package. If so, the isIgnored function won't pick up when the parent of the second package becomes non-ignored due to a dependency that is processed later. This is the cause of bug #2468 and bug #2263 (and probably #2421 too, though I haven't tested that one)This happens with roughly this dependency tree:
dev A -> B (1)
dev A -> C -> B (2)
normal D -> E -> F -> C (3)
Changing the isIgnored function to call both the existing and the deduped isIgnored will cause a stack overflow to happen in the test of 2263, so that solution is out.
To fix this, I have opted to remove the isIgnored function calls, and replace it with a 2 booleans, one to keep the current ignore status and one note wither inheritance is allowed (so it works just like the function call solution). With a processing step after seeding the hoister will then mark all dependencies of non-ignored packages as non-ignored too.
Test plan
I have enabled the test from #2263, it runs without errors. Also, I have run a production-only install on the package.json from #761 (comment), and abbrev is installed there too.
yarn run testalso finishes without any failures.The algorithm for marking the hoistmanifests will only visit non-ignored ones, and only once, so can't lead to an infinite loop.