Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: latest
Choose a base branch
from

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Jul 10, 2025

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 G-Rath marked this pull request as ready for review July 10, 2025 21:04
@G-Rath G-Rath requested a review from a team as a code owner July 10, 2025 21:04
@G-Rath G-Rath force-pushed the prune-optional-peer branch from 7127150 to efd74f7 Compare July 10, 2025 21:25
@G-Rath
Copy link
Contributor Author

G-Rath commented Jul 10, 2025

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
Copy link
Member

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.

@wraithgar wraithgar self-assigned this Jul 10, 2025
@G-Rath
Copy link
Contributor Author

G-Rath commented Jul 11, 2025

@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:

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 😞

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

Successfully merging this pull request may close these issues.

[BUG] uninstalling an optional peer dep doesnt remove it
2 participants