Fix #1435 (failing dependencies of optional dependencies) #2811
Conversation
Mark all dependencies of optional dependencies as themselves optional, so that if they fail, the whole install process doesn't fail.
| @@ -0,0 +1 @@ | |||
| yarn-offline-mirror=./mirror-for-offline | |||
There was a problem hiding this comment.
Do you need offline mirror and .tgz files for the test?
I think you could be fine just using file: dependencies for tests
There was a problem hiding this comment.
No, I don't think it's needed. I was trying to make the changes fit with their surroundings and mimic this test which uses a tgz file.
I'm happy to convert the new test to using file: dependencies. Should I also convert the one I modeled it after while I'm at it?
There was a problem hiding this comment.
Yeah, let's make it simpler in both places.
.tgz file are harder to review
| @@ -281,7 +281,8 @@ export default class PackageRequest { | |||
| promises.push(this.resolver.find({ | |||
| pattern: depPattern, | |||
| registry: remote.registry, | |||
| optional: false, | |||
| // dependencies of optional dependencies should themselves be optional | |||
| optional: this.optional, | |||
There was a problem hiding this comment.
What happens if a dependency is optional in one subtree and is not optional in another:
A -> (optional) B -> C
D -> C
Would it mark C as optional even though D requires it as a regular dependency?
Could you add a test for that to make sure we don't break it?
There was a problem hiding this comment.
Pretty sure that would work fine because of this code but I'll add a unit test to prove it.
There was a problem hiding this comment.
Hmm...couldn't your scenario be simplified to
(optional) A -> C
B -> C
and still prove what you are concerned about? I'm not seeing why the first chain has to have a depth of 3 -- am I missing something?
There was a problem hiding this comment.
Just a gut feeling, we had problems with deeper chains of dependency :)
Probably 2 layers should be good enough
Add a test to make sure code marking sub-dependencies as optional doesn't do so when another dependency marks it as non-optional, and convert a test (plus a pre-existing one) to use file: URIs instead of offline mirroring, for readability.
|
@bestander feedback addressed. Let me know if you'd like me to squash the commits. |
|
Thanks for the whole bunch of tests! |
Summary
Mark all dependencies of optional dependencies as themselves optional, so that if they fail, the whole install process doesn't fail.
Test plan
I implemented integration tests for the install command for the cases of the install script failing, and platform incompatibility. I didn't find any similar tests for the add command, and since it appears that the code change is in a code path common to both, I though that would be fine.
I ran a number of manual test cases:
yarn add electron-installer-dmg(has optional dependencyappdmg, which has normal dependencymacos-alias, both of which are"os": [ "darwin" ])yarn add electron-installer-dmg, copypackage.jsonandyarn.lockto linux and runyarn installyarn add appdmg, copypackage.jsonandyarn.lockto linux and runyarn installAll behaved as expected.
NOTE: This is a replacement for #1438