fix: don't trash optional dependencies that fail due to incompatible platform

[paulrutter]

This PR is a proposed solution for multiple reported issues regarding failed optional dependencies that are platform specific.
See #4828, #7961 and possibly related PR #8077.

It can be reproduced as follows:

• mkdir test && cd test
• npm init -y
• npm i --verbose rollup
• Read package-lock.json - observe 18 OS/CPU@rollup/* package variants are listed
• rm package-lock.json
• npm i --verbose
• Read package-lock.json - observe only 1 OS/CPU @rollup/* package variant is now listed for the current platform

With this PR, the package-lock.json will remain to contain the initial 18 rollup related dependencies, as any optional failure due to incompatible platform (EBADPLATFORM) will not lead to trashing the node.

The side-effect of this is that all platform specific dependencies will be present on all platforms (just an empty directory where the platform doesn't match). But it does solve the issue at hand.

I would need help writing a proper unit test for this scenario though. With the steps below, i did manage to test my PR and see that it has the desired effect:

• node . i --verbose rollup
• Read package-lock.json - observe 18 OS/CPU@rollup/* package variants are listed
• rm package-lock.json
• node . i --verbose
• Read package-lock.json - observe 18 OS/CPU@rollup/* package variants are listed still

It will show this in the logging (on my Windows based machine):

npm verbose reify skip trashing optional dependency due to platform mismatch C:\git\npm-cli\node_modules\@rollup\rollup-win32-ia32-msvc
npm verbose reify skip trashing optional dependency due to platform mismatch C:\git\npm-cli\node_modules\@rollup\rollup-win32-arm64-msvc
npm verbose reify skip trashing optional dependency due to platform mismatch C:\git\npm-cli\node_modules\@rollup\rollup-linux-x64-musl
npm verbose reify skip trashing optional dependency due to platform mismatch C:\git\npm-cli\node_modules\@rollup\rollup-linux-x64-gnu
npm verbose reify skip trashing optional dependency due to platform mismatch C:\git\npm-cli\node_modules\@rollup\rollup-linux-s390x-gnu
npm verbose reify skip trashing optional dependency due to platform mismatch C:\git\npm-cli\node_modules\@rollup\rollup-linux-riscv64-gnu
npm verbose reify skip trashing optional dependency due to platform mismatch C:\git\npm-cli\node_modules\@rollup\rollup-linux-powerpc64le-gnu
npm verbose reify skip trashing optional dependency due to platform mismatch C:\git\npm-cli\node_modules\@rollup\rollup-linux-loongarch64-gnu
npm verbose reify skip trashing optional dependency due to platform mismatch C:\git\npm-cli\node_modules\@rollup\rollup-linux-arm64-musl
npm verbose reify skip trashing optional dependency due to platform mismatch C:\git\npm-cli\node_modules\@rollup\rollup-linux-arm64-gnu
npm verbose reify skip trashing optional dependency due to platform mismatch C:\git\npm-cli\node_modules\@rollup\rollup-linux-arm-musleabihf
npm verbose reify skip trashing optional dependency due to platform mismatch C:\git\npm-cli\node_modules\@rollup\rollup-linux-arm-gnueabihf
npm verbose reify skip trashing optional dependency due to platform mismatch C:\git\npm-cli\node_modules\@rollup\rollup-freebsd-x64
npm verbose reify skip trashing optional dependency due to platform mismatch C:\git\npm-cli\node_modules\@rollup\rollup-freebsd-arm64
npm verbose reify skip trashing optional dependency due to platform mismatch C:\git\npm-cli\node_modules\@rollup\rollup-darwin-x64
npm verbose reify skip trashing optional dependency due to platform mismatch C:\git\npm-cli\node_modules\@rollup\rollup-darwin-arm64
npm verbose reify skip trashing optional dependency due to platform mismatch C:\git\npm-cli\node_modules\@rollup\rollup-android-arm64
npm verbose reify skip trashing optional dependency due to platform mismatch C:\git\npm-cli\node_modules\@rollup\rollup-android-arm-eabi
npm verbose reify skip trashing optional dependency due to platform mismatch C:\git\npm-cli\node_modules\fsevents

[paulrutter]

@wraithgar would you be able to take a look at this PR? Thanks 👍

[paulrutter]

@owlstronaut thanks for running the test suite. So, in this case, would it make sense to adjust the tests? Seems like these are incorrect, if we want to retain the entries in the lock file.

About code coverage, where would i need to add or extend the tests for build-ideal-tree? See also #8127 (comment)

[owlstronaut]

@owlstronaut thanks for running the test suite. So, in this case, would it make sense to adjust the tests? Seems like these are incorrect, if we want to retain the entries in the lock file.

About code coverage, where would i need to add or extend the tests for build-ideal-tree? See also #8127 (comment)

Sure! Yeah, in this case we would need to update the snapshot for the new expectations from reify, and for the isolated-mode, the 2 failing tests probably need a helper similar to setupRequire that will expect to find the folders, but find them empty.

workspaces/arborist/test/arborist/build-ideal-tree.js will need a test added for the code coverage.

I still have to leave your questions about the "right thing" to do open to @wraithgar, I've been on the team less than a month 😸

[paulrutter]

Working through the tests and comments now, hope to push something this evening.

[paulrutter]

@owlstronaut Could you run the test suite again? I fixed all the snapshot and failing tests.

Besides, i removed the code in build-ideal-tree, as it never seems to hit (in unit tests, but also not when building locally). The code in build-ideal-tree is not entirely clear to me, as i cannot figure out if loadFailures could ever contain a EBADPLATFORM error or not. In unit tests, it's always E404 or ETARGET, never EBADPLATFORM. I tried adding a testcase for this, but still didn't get EBADPLATFORM. Hence i removed the code in build-ideal-tree.

With these changes, the original issue is still verified to be fixed, see the changed snapshots.
The remaining question is if this is the desired approach to this issue.

[jakebailey]

Am I right to say that the new behavior would be to leave empty directories for all of the optional deps that did not succeed? Hopefully there's a way to avoid this, since that would leave a load of empty dirs for a lot of native packages, and potentially confuse code which is scanning for these dirs to know which one to load for the current platform...

[paulrutter]

That is correct indeed. This is used to rebuild the proper package-json.lock file based on the node_modules directory.
I don't know enough of NPM to surface a better solution, but maybe others have ideas on how this can prevented?

Instead of the empty directory, it could also keep a list of failed optionals in a separate file or something, but that would require far more changes i would assume.

[jakebailey]

That's really unfortunate; I thought that npm stored a file inside node_modules that described the layout? It seems like a bad idea to rely on an empty dir existing, and I really do think it'll cause something to break.

[paulrutter]

Yes, i can imagine breakage if packages rely on directory scanning without verifying the contents.

If this is not the desired approach, i would need guidance from the maintainers on what would be the best way forward.

[owlstronaut]

Yep, we'll have @wraithgar weigh in