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): node.target can be null #7065

Open
wants to merge 1 commit into
base: latest
Choose a base branch
from

Conversation

ljharb
Copy link
Collaborator

@ljharb ljharb commented Dec 8, 2023

@ljharb ljharb requested a review from a team as a code owner December 8, 2023 23:15
@ljharb ljharb added Bug thing that needs fixing Release 10.x labels Dec 8, 2023
@ljharb
Copy link
Collaborator Author

ljharb commented Dec 8, 2023

a node 16 test failed but since node 16 isn't supported anymore, i assume those tests can be dropped (and node 21 added)?

@ljharb ljharb force-pushed the arborist-no-target branch 2 times, most recently from ba5978b to ba53945 Compare January 18, 2024 22:54
@wraithgar wraithgar added the pr: needs tests requires tests before merging label Jan 19, 2024
@lukekarrys lukekarrys self-assigned this Jan 29, 2024
@lukekarrys
Copy link
Member

I am still planning to add a failing test case for this in smoke-tests, but I do think this fix looks good.

@ljharb
Copy link
Collaborator Author

ljharb commented Feb 2, 2024

Thanks! I'm looking forward to it landing and being unblocked, and if there's anything concrete I can do to hurry it along I'd love pointers.

@dmnsgn
Copy link

dmnsgn commented Feb 21, 2024

I am getting a related Cannot destructure property 'package' of 'node.target' as it is null. but coming from another point in arborist's code than the above fix:

const { package: pkg, hasInstallScript } = node.target

Using arborist@7.3.1 in node@v21.6.2 for a package with some dependencies as file: symlinks (with some being inter-dependent).

Stacktrace:

at [addToBuildSet] (/Users/myname/my-package/node_modules/@npmcli/arborist/lib/arborist/rebuild.js:270:22)
at [buildQueues] (/Users/myname/my-package/node_modules/@npmcli/arborist/lib/arborist/rebuild.js:210:41)
at [build] (/Users/myname/my-package/node_modules/@npmcli/arborist/lib/arborist/rebuild.js:182:29)
at Arborist.rebuild (/Users/myname/my-package/node_modules/@npmcli/arborist/lib/arborist/rebuild.js:100:25)
at async [reifyPackages] (/Users/myname/my-package/node_modules/@npmcli/arborist/lib/arborist/reify.js:252:11)
at async Arborist.reify (/Users/myname/my-package/node_modules/@npmcli/arborist/lib/arborist/reify.js:171:5)

@lukekarrys
Copy link
Member

@dmnsgn That looks like a different bug. Can you open a new issue about it including your package.json with the interdependent symlinks?

@dmnsgn
Copy link

dmnsgn commented Feb 27, 2024

@dmnsgn That looks like a different bug. Can you open a new issue about it including your package.json with the interdependent symlinks?

Sorry, I am having a hard time making a reduced case but a clue for solving the issue is removing a leftover package-lock.json in a workspace.
Let me just add that I was also getting Cannot read properties of undefined (reading 'target') when running npm query '*:missing' and the following stack (workspace.target being undefined at execWorkspaces):

npm info using npm@10.2.4
npm info using node@v21.6.2
// ...
npm verb stack TypeError: Cannot read properties of undefined (reading 'target')
npm verb stack     at Query.execWorkspaces (/usr/local/lib/node_modules/npm/lib/commands/query.js:104:33)
npm verb stack     at async module.exports (/usr/local/lib/node_modules/npm/lib/cli-entry.js:61:5)

Not sure it is worth opening an issue if I can't provide repro easily but maybe the above might help someone or "leftover package-lock" might ring a bell to someone who's worked on this.

@dmnsgn
Copy link

dmnsgn commented Apr 11, 2024

Another clue is: one of the workspace was dependent on another workspace but its dependency version was different.

Example: a is dependent on b@0.0.1, but b package.json version is 0.0.2.

.
+-- package.json
`-- packages
   +-- a@0.0.1
   |   `-- package.json (depends on b@0.0.1)
   `-- b@0.0.2
       `-- package.json

That solves it for me so I'll stop here and open another issue if I encounter these errors again. Hope that helps someone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing pr: needs tests requires tests before merging Release 10.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] npm install fails in my project since v7
4 participants