Prevent optional subdependencies from being installed with --ignore-optional #3976
Conversation
| {hint: 'optional', optional: true}, | ||
| !this.flags.ignoreOptional, | ||
| ); | ||
| pushDeps('optionalDependencies', projectManifestJson, {hint: 'optional', optional: true}, true); |
There was a problem hiding this comment.
Because the ignore flag isn't managed anymore by this portion of the code - we do it directly in the hoister instead, so that we can operate recursively.
There was a problem hiding this comment.
Gotcha. May be we should remove that flag in a follow-up since it is only used by devDependencies and is confusing a bit.
There was a problem hiding this comment.
isUsed is also used to remove the package from consideration in flat mode, so I wouldn't remove it (if another package requires an incompatible version, this would create a conflict). (never mind, the changes to getFlatHoistedTree fix that :-) )
src/package-hoister.js
Outdated
| @@ -142,15 +146,15 @@ export default class PackageHoister { | |||
| // | |||
| let parentParts: Parts = []; | |||
| const isIncompatible = ref.incompatible; | |||
| let isRequired = isDirectRequire && !ref.ignore && !isIncompatible; | |||
| let isRequired = isDirectRequire && !ref.ignore && !isIncompatible && (!ref.optional || !this.ignoreOptional); | |||
There was a problem hiding this comment.
Better to write as !(ref.optional && this.ignoreOptional) since makes the intent clearer. Also possibly fewer instructions :P
There was a problem hiding this comment.
That's purely stylistic ;)
There was a problem hiding this comment.
Not really stylistic. One reads "if ref is not optional or we should not ignore optionals" and the other one reads "if NOT (ref is optional and we should ignore optionals)". Human brains are bad at double negations ;)
src/package-hoister.js
Outdated
|
|
||
| if (parent) { | ||
| if (!this.tree.get(parent.key)) { | ||
| return null; | ||
| } | ||
| // non ignored dependencies inherit parent's ignored status | ||
| // parent may transition from ignored to non ignored when hoisted if it is used in another non ignored branch | ||
| if (!isDirectRequire && !isIncompatible && parent.isRequired) { | ||
| if (!isDirectRequire && !isIncompatible && parent.isRequired && (!ref.optional || !this.ignoreOptional)) { |
There was a problem hiding this comment.
Same as above. Also, may convert this to a function instead of repeating the same logic in multiple places?
src/package-hoister.js
Outdated
| depinfo && | ||
| !depinfo.isRequired && | ||
| !depinfo.isIncompatible && | ||
| (!depinfo.pkg._reference || !depinfo.pkg._reference.optional || !this.ignoreOptional) |
There was a problem hiding this comment.
Yeah, we definitely need to extract this logic out.
There was a problem hiding this comment.
Yeah I agree, but there was some subtle changes in the conditions so I aimed to make the minimal change required and revisit if needed. I'll give it a second look.
There was a problem hiding this comment.
We can defer this to a follow-up since it's not the main goal of the PR anyways.
Fix #2666
@blexrob Would you mind reviewing this PR? You worked a bit on the hoister in #3465, so you might have some additional insight :)