Skip to content

fix(arborist): don't checkEngine/checkPlatform for extraneous deps #8310

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 2 commits into
base: latest
Choose a base branch
from

Conversation

YodaDaCoda
Copy link

Fixes #7622

The linked issue describes the problem in detail, I won't replicate that here.

@YodaDaCoda YodaDaCoda requested a review from a team as a code owner May 20, 2025 11:07
@owlstronaut
Copy link
Contributor

@YodaDaCoda Are you able to add a test that would fail before the fix?

@YodaDaCoda YodaDaCoda changed the title fix(arborist): don't checkEnging/checkPlatform for extraneous deps fix(arborist): don't checkEngine/checkPlatform for extraneous deps May 21, 2025
@YodaDaCoda
Copy link
Author

YodaDaCoda commented May 21, 2025

I have made an attempt... but it doesn't work how I expect it to. The test passes whether or not the fix is in place (i.e. it doesn't throw). I need help from someone more familiar with the codebase to figure out how to properly trigger the condition.

@wraithgar
Copy link
Member

Strict mode throws. You can pass in the values and set strict to true. A good example is here

@YodaDaCoda
Copy link
Author

I worded that very poorly. It probably would throw, if the problematic dep wasn't being pruned. Not clear to me why it's pruned in the test, but not in Real Life. The package.json / package-lock.json probably need to be tweaked somehow.

It's the checkPlatform() method that I've experienced causing the error, not checkEngine(), so the engineStrict option doesn't help.

That example you've shared does help me clean up the test somewhat though, thanks.

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] npm ci erroneously installs optional OS-constrained transitive dependency through direct shrinkwrap dependency
3 participants