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): dependencies from registries with a peerDependency on a workspace #6193

Merged
merged 2 commits into from
Mar 2, 2023

Conversation

ixalon
Copy link
Contributor

@ixalon ixalon commented Feb 23, 2023

Currently if you attempt to install a package from a registry that has a peer dependency on a package published from a workspace in your local project, you are presented with the error (even if all specs match and there are no version conflicts):

44 verbose stack TypeError: Cannot set property 'parent' of null
44 verbose stack     at Arborist.[nodeFromEdge] (/Users/chris/.nvm/versions/node/v14.18.3/lib/node_modules/npm/node_modules/@npmcli/arborist/lib/arborist/build-ideal-tree.js:1129:17)
44 verbose stack     at async Arborist.[loadPeerSet] (/Users/chris/.nvm/versions/node/v14.18.3/lib/node_modules/npm/node_modules/@npmcli/arborist/lib/arborist/build-ideal-tree.js:1373:23)
44 verbose stack     at async Arborist.[buildDepStep] (/Users/chris/.nvm/versions/node/v14.18.3/lib/node_modules/npm/node_modules/@npmcli/arborist/lib/arborist/build-ideal-tree.js:976:11)
44 verbose stack     at async Arborist.buildIdealTree (/Users/chris/.nvm/versions/node/v14.18.3/lib/node_modules/npm/node_modules/@npmcli/arborist/lib/arborist/build-ideal-tree.js:218:7)
44 verbose stack     at async Promise.all (index 1)
44 verbose stack     at async Arborist.reify (/Users/chris/.nvm/versions/node/v14.18.3/lib/node_modules/npm/node_modules/@npmcli/arborist/lib/arborist/reify.js:153:5)
44 verbose stack     at async Install.exec (/Users/chris/.nvm/versions/node/v14.18.3/lib/node_modules/npm/lib/commands/install.js:156:5)
44 verbose stack     at async module.exports (/Users/chris/.nvm/versions/node/v14.18.3/lib/node_modules/npm/lib/cli.js:78:5)

This is an edge case, but something we've come across frequently due to having multiple repos publishing packages (e.g. strongly typed clients) where occasionally a cyclic dependency exists within the type system (which TypeScript can handle once we have the packages installed).

This does not affect packages referred to by path as these are handled by: https://github.com/npm/cli/blob/latest/workspaces/arborist/lib/arborist/build-ideal-tree.js#L1236-L1238

The issue appears to be https://github.com/npm/cli/blob/latest/workspaces/arborist/lib/arborist/build-ideal-tree.js#L1245-L1247 where we find a suitable existing node, but then immediately throw it away, returning the unresolved edge.to.

I believe this is a typo and returning existingNode is the desired behaviour? I've added a test case that fails without this change.

@ixalon ixalon requested a review from a team as a code owner February 23, 2023 13:32
@ixalon ixalon requested review from wraithgar and removed request for a team February 23, 2023 13:32
@ixalon ixalon changed the title fix(arborist): support dependencies from registries with a peer dependency on a workspace fix(arborist): dependencies from registries with a peerDependency on a workspace Feb 23, 2023
Copy link
Contributor

@fritzy fritzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fritzy fritzy merged commit 962a12e into npm:latest Mar 2, 2023
@github-actions github-actions bot mentioned this pull request Mar 2, 2023
@ixalon ixalon deleted the fix/workspace-cyclic-peer-deps branch March 7, 2023 07:43
@github-actions github-actions bot mentioned this pull request Oct 6, 2023
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.

2 participants