Skip to content

fix duplicate dependency analysis for workspace links#201

Closed
tristanmanchester wants to merge 2 commits intoe18e:mainfrom
tristanmanchester:codex/fix-workspace-link-duplicate-analysis
Closed

fix duplicate dependency analysis for workspace links#201
tristanmanchester wants to merge 2 commits intoe18e:mainfrom
tristanmanchester:codex/fix-workspace-link-duplicate-analysis

Conversation

@tristanmanchester
Copy link
Copy Markdown

Summary

  • ignore lockfile package entries that do not have a resolved version during duplicate dependency analysis
  • add a regression test covering npm workspace link entries in package-lock data

Root cause

npm workspaces can appear in package-lock parsing as linked packages without a version. The duplicate dependency analyzer treated every package entry as a concrete installed version and passed an undefined version string into node:util styleText, which crashed analysis on Node 24.

Validation

  • npm test -- src/test/duplicate-dependencies.test.ts
  • npm run build
  • linked the patched CLI into /Users/tristan/Projects/react-native-dotgrid and verified analyze now completes instead of crashing

@tristanmanchester tristanmanchester marked this pull request as ready for review March 31, 2026 14:57
@tristanmanchester tristanmanchester changed the title [codex] fix duplicate dependency analysis for workspace links fix duplicate dependency analysis for workspace links Mar 31, 2026
Copy link
Copy Markdown
Collaborator

@dreyfus92 dreyfus92 left a comment

Choose a reason for hiding this comment

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

hey @tristanmanchester thank you for taking the time in working on this 😄, i left a 🤏🏻 nit.

Comment on lines +10 to +17
function hasResolvedVersion(
pkg: Pick<ParsedLockFile['packages'][number], 'version'>
): pkg is Pick<ParsedLockFile['packages'][number], 'version'> & {
version: string;
} {
return typeof pkg.version === 'string' && pkg.version.length > 0;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this is logically fine but a bit hard to read, and tying it to Pick<ParsedLockFile['packages'][number], 'version'> is redundant in practice (packages is ParsedDependency[] in lockparse). i’d drop the helper and inline an early continue, e.g.

if (typeof pkg.version !== 'string' || pkg.version.length === 0) continue

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yep makes sense! i wanted to keep the runtime guard because npm workspace links can show up without a version even though lockparse types it as string, but agreed the helper can be removed. I inlined the early continue instead

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Apr 1, 2026

Open in StackBlitz

npm i https://pkg.pr.new/@e18e/cli@201

commit: e897be9

@43081j
Copy link
Copy Markdown
Contributor

43081j commented Apr 1, 2026

hmm while this does fix it, i think we actually have our types wrong in lockparse and should just check link.

currently, we have:

https://github.com/43081j/lockparse/blob/6ac3a2b7cf27b0ab9fb0a6bd9d07caeed8ece0f4/src/parsers/npm.ts#L24

which references this:

https://github.com/43081j/lockparse/blob/6ac3a2b7cf27b0ab9fb0a6bd9d07caeed8ece0f4/src/parsers/npm.ts#L11-L19

but that isn't entirely accurate since workspace packages show up like this in the actual JSON:

    "node_modules/@43081j/sdk": {
      "resolved": "packages/sdk",
      "link": true
    },

so i think the better solution here is that lockparse has this type instead:

  packages: Record<string, NpmLockFilePackage | NpmLockFilePackageLink>;

which is a new type:

interface NpmLockFilePackageLink {
  resolved: string;
  link: true;
}

there's no real reference for this from npm themselves afaik so we're doing a bit of guesswork here. but then the change in this PR could continue checking version or could check !pkg.link.

@tristanmanchester
Copy link
Copy Markdown
Author

Hmm yes you’re right about the underlying issue being in lockparse.

npm does seem to emit workspace entries like { resolved: "…", link: true } in package-lock.json for workspaces, but lockparse currently types packages as a single NpmLockFilePackage shape with version: string, and then maps those into ParsedDependency without preserving link.

Also I checked for a link: true package the parsed object ends up with version as undefined and no link field at all, so in this PR against the current lockparse output checking version works, but checking !pkg.link is not actually possible unless lockparse changes first, I think..

So I think this PRs guard is still the practical fix here, and the better long-term fix is upstream in lockparse, let me know what you think

@43081j
Copy link
Copy Markdown
Contributor

43081j commented Apr 1, 2026

yes i am saying lockparse should change first 👍

@tristanmanchester
Copy link
Copy Markdown
Author

Ah I misunderstood, make sense, I'll close :)

@tristanmanchester
Copy link
Copy Markdown
Author

Ah I misunderstood, make sense, I'll close :)

1 similar comment
@tristanmanchester
Copy link
Copy Markdown
Author

Ah I misunderstood, make sense, I'll close :)

@tristanmanchester tristanmanchester deleted the codex/fix-workspace-link-duplicate-analysis branch April 2, 2026 11:11
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.

3 participants