Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

skip optional deps with mismatched platform #231

Merged
merged 3 commits into from Feb 18, 2021

Conversation

nlf
Copy link
Contributor

@nlf nlf commented Feb 16, 2021

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

Comment on lines +451 to +452
const { npmVersion, nodeVersion } = this.options
const p = Promise.resolve()
.then(() => {
Copy link
Contributor

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 .thens?

Copy link
Contributor Author

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

Copy link
Contributor

@isaacs isaacs left a 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.

// of the mismatches
if (node.optional) {
checkEngine(node.package, npmVersion, nodeVersion, this[_force])
checkPlatform(node.package, this[_force])
Copy link
Contributor

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.

Copy link
Contributor

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
@isaacs isaacs force-pushed the nlf/skip-optional-deps-with-mismatched-platform branch from a803be4 to 4a3322d Compare February 18, 2021 22:26
@isaacs isaacs merged commit 4a3322d into main Feb 18, 2021
isaacs added a commit to npm/cli that referenced this pull request Feb 18, 2021
* [#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))
@nlf nlf mentioned this pull request Feb 22, 2021
@wraithgar wraithgar deleted the nlf/skip-optional-deps-with-mismatched-platform branch April 22, 2021 17:46
@Brooooooklyn
Copy link

So how could I install dependencies for the other platform now? For example build macOS arm64 electron App on macOS x64 machine?

Related:

napi-rs/node-rs#376
swc-project/swc#2898

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] cpu and os properties not respected
4 participants