-
Notifications
You must be signed in to change notification settings - Fork 64
skip optional deps with mismatched platform #231
Conversation
const { npmVersion, nodeVersion } = this.options | ||
const p = Promise.resolve() | ||
.then(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function might be a bit cleaner if you use async [_reifyNode] (node) {
, and then use await
instead of all the nested .then
s?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it might, but to be consistent with the rest of this code base i left it as-is. we should refactor this repo to be more await-y at some point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just omit if it's optional. don't force past it. users can install as a non-optional dep with --force
in that case.
lib/arborist/reify.js
Outdated
// of the mismatches | ||
if (node.optional) { | ||
checkEngine(node.package, npmVersion, nodeVersion, this[_force]) | ||
checkPlatform(node.package, this[_force]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be a bad idea to respect force
here, since we're fine with it not being installed anyway.
Since people use --force
to override peer conflicts, it could easily result in incompatible optional binary modules being installed, which was the whole thing we wanted to fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ie, I think npm v6 was wrong in this case, and probably only respected --force
in those checks because it only did them in one place, regardless of optionality.
…ll and uses a completely invalid os also added an actual assertion to a build-ideal-tree test, and removed a test skip that we no longer need
a803be4
to
4a3322d
Compare
* [#1875](#1875) [npm/arborist#230](npm/arborist#230) Set default advisory `severity`/`vulnerable_range` when missing from audit endpoint data ([@isaacs](https://github.com/isaacs)) * [npm/arborist#231](npm/arborist#231) skip optional deps with mismatched platform or engine ([@nlf](https://github.com/nlf)) * [#2251](#2251) Unpack shrinkwrapped deps not already unpacked ([@isaacs](https://github.com/isaacs), [@nlf](https://github.com/nlf)) * [#2714](#2714) Do not write package.json if nothing changed ([@isaacs](https://github.com/isaacs)) * [npm/rfcs#324](npm/rfcs#324) Prefer peer over prod dep, if both specified ([@isaacs](https://github.com/isaacs)) * [npm/arborist#236](npm/arborist#236) Fix additional peerOptional conflict cases ([@isaacs](https://github.com/isaacs))
So how could I install dependencies for the other platform now? For example build macOS arm64 electron App on macOS x64 machine? Related: |
this solves for the use case where a package has platform dependent child packages and lists each of them as optionalDependencies such that we will only install the optional dependencies that do not fail platform and engine checks.
this follows the same behavior as npm 6 where
--force
will ignore the checks and install all the optional dependencies.References
Fixes npm/cli#2707