fix: prune optional peer dependencies that are no longer explicitly depended on

[G-Rath]

Currently optional peer dependencies are retained so long as at least one package in the tree has them as an optional peer dependency, meaning once added they become non-optional even though npm does actually seem to mark such dependencies.

To resolve this, I've modified the pruning logic to check for nodes that are both optional and peer, in addition to extraneous nodes.

References

Resolves #4737

[G-Rath]

so the Windows tests are failing because my new test results in a request for the dedent package and if I've understood all the other tests they don't result in a package after pruning so they avoid needing to do any mocking - I don't mind removing dedent from my test too if thats ok; otherwise I'll need guidance on how to address this as my attempts so far (mainly around mocking out the dependency / registry) have failed.

I've kept the adding of the tgz file in case that will be useful - I'm also not sure why this fails only on Windows 😕

[wraithgar]

I'm also not sure why this fails only on Windows

Yeah this is a GOOD thing cause answering this question will help us know if this PR is right.

[G-Rath]

@wraithgar so I've spent a few hours trying to dig into this without much luck, though I'm still pretty sure the failure is about the test setup rather than the fix being wrong - if I revert the fix, then all the tests fail with the same kind of error though there's more of them because it's resolving about 20 more packages.

I think the issue is something on the lines of dependencies (not) being cached - the error gets raised when calling this:

cli/workspaces/arborist/lib/arborist/reify.js

Lines 799 to 803 in c457c75

	await pacote.extract(res, node.path, {
	...this.options,
	resolved: node.resolved,
	integrity: node.integrity,
	})

I also feel like when I revert the fix, the Windows test fail about different packages e.g. it complains about json-parse-even-better-errors whereas the Linux tests don't which could make sense if its related to caches and such because that package is a dependency used by NPM.

I don't think I'll be able to dig into this much further by myself I'm afraid as I've not found any solid difference between Windows and Linux 😞

[G-Rath]

I hadn't realized that node_modules/pacote is actually included in git, so that made it easy to patch in some logging and doing so shows this is because of caching:

What's interesting is that this patching also applies to the rest of the CI so I can see that dedent is being installed as part of the "reset deps" step which I assume is how it ends up in the cache resulting in the Linux and macOS CIs running...

[G-Rath]

ok so the issue is that the wrong cache directory is being used on Windows:

pacote looks in the home directory for the cache, but on Windows the cache is being stored in C:\npm

[G-Rath]

@wraithgar I've got the tests passing in a manner that you might not be too impressed by, but that I think at least shows the issue isn't with my change

[wraithgar]

The workaround is very intentional and well commented. It's always these one-line tweaks that get so far off the rails when it comes to testing. Thank you for seeing this through.

[wraithgar]

It's possible the mock npm fixture has a quirk in windows with this caching but fixing that is way outside the scope of this PR.

[lkraav]

Is there a chance of this getting backported?

[wraithgar]

@lkraav we typically don't actively do backports but this would probably be something we would consider if a PR was made against the v10 branch

[G-Rath]

I have opened #8449 backporting this, unfortunately CI for Linux and macOS is failing - since (ironically) Windows is passing, I'm guessing it's cache related and I wasn't able to figure out the root cause for this PR.

My very hacky work for exploring this is in https://github.com/G-Rath/cli/tree/prune-optional-peer2 if someone wants to have a crack at digging further - I'm not sure if I'll have the time this week

[baileympearson]

Hey @G-Rath , I'm pretty sure this change broke our CI. I don't think this is a desirable way to fix the linked issue because with this change, it is no longer possible to install a peer optional dependency that isn't tracked as a dependency. This seems like a regression. Can we revert this change?

Context: I maintain the mongodb pacakge, which has a number of peer optional dependencies. Some of these are native packages with native addons. One of these dependencies (kerberos) doesn't support the full platform matrix that the mongodb package supports. So we don't track it as a dependency because if we did npm i would fail when we run our test suite on these platforms. Instead, we explicitly install this dependency before we use it in our test suite (npm i && npm i kerberos). On npm 11.5.0+, explicitly installing the dependency succeeds (exit code 0) but the dependency is not present (node -p "require('kerberos')" throws an error because kerberos was never installed).

[billyjanitsch]

Suppose my package A depends on package B which peer-depends on package C which optionally depends on package D.

This change causes D to be removed from package-lock.json and node_modules.

That doesn't seem correct. D should still be installed.

I think this PR has conflated the concept of optional peer dependencies with the concepts of peer dependencies + optional dependencies.

[silverwind]

This change broke my CI as well where I verify that npm generates no diff in the lockfile. With v11.4.2 such optional peer dependencies were present in the lockfile, but since v11.5.0, they are now absent.

The change is fine in itself I guess, but I wonder if npm could better communicate such lockfile-breaking changes better.