Skip to content
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

fix(arborist): safeguard against null node.target in flag calculation #7579

Merged
merged 7 commits into from
Jul 1, 2024

Conversation

AmirSa12
Copy link
Contributor

@AmirSa12 AmirSa12 commented Jun 1, 2024

If a node represents a symbolic link or a file dep (node.isLink is true), its target is expected to reference another node in the dependency tree. If the linking is not done correctly or is incomplete, node.target might be null.

in this PR, a null check is added to ensure node.target is not null or before proceeding, which will prevent causing errors like:
npm error Cannot set properties of null (setting 'peer')

References

Related to #7065,
Fixes #6622, #5007,
Closes #6622, #5007

@AmirSa12 AmirSa12 requested a review from a team as a code owner June 1, 2024 06:30
@wraithgar
Copy link
Member

This needs a test. It's one thing to say the code can get into this situation, it's another to know why and show it in tests.

@wraithgar
Copy link
Member

If I'm understanding this PR correctly, when npm encounters an invalid tree (i.e. an incomplete linking) it will now ignore it? I don't know if this is what we want. Folks are going to have an even tougher time figuring out why things aren't working in this situation. npm will not give any errors, but their code is not going to work properly when requiring those dependencies.

@ljharb
Copy link
Contributor

ljharb commented Jun 5, 2024

When I run into this category of problem, everything about my code works fine when i patch npm to not throw on a null node.target, fwiw.

@wraithgar
Copy link
Member

Is there any overlap here with #7588?

@AmirSa12
Copy link
Contributor Author

AmirSa12 commented Jun 5, 2024

If I'm understanding this PR correctly, when npm encounters an invalid tree (i.e. an incomplete linking) it will now ignore it? I don't know if this is what we want. Folks are going to have an even tougher time figuring out why things aren't working in this situation. npm will not give any errors, but their code is not going to work properly when requiring those dependencies.

I'm not sure if I can call that invalid tree, because everything about the code will work fine when we apply the patch or, use pnpm or yarn. the target can be null in some deps, mostly private packages and local deps. check out the issues I mentioned in this PR.
there is also a failing test I added.

@wraithgar
Copy link
Member

Thanks for your patience on this. I think it's ready to land.

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] Cannot set properties of null (setting 'peer')
3 participants