refactor(parsers): drive lockfile handling from one precedence list - #1130
Open
sonukapoor wants to merge 2 commits into
Open
refactor(parsers): drive lockfile handling from one precedence list#1130sonukapoor wants to merge 2 commits into
sonukapoor wants to merge 2 commits into
Conversation
loadPackages handled the same five lockfiles twice, once for the repo root and once for nested discovery, as two if-chains about 100 lines apart holding 12 near-identical ScanInput literals. chooseBestLockfile then kept a third, separate ordering of its own. Three copies meant three chances to disagree, and they did. Add LOCKFILE_PRIORITY to constants.ts as the single source of truth, and a LOCKFILE_HANDLERS table keyed by filename saying how to read each one. Root detection, nested discovery and chooseBestLockfile all walk that one list, so precedence cannot drift again. Keying the table by filename means the compiler requires a handler for every entry. Fixes two precedence bugs this exposed: - npm-shrinkwrap.json lost to package-lock.json for nested lockfiles while winning at the root, because chooseBestLockfile never scored it and fell through to its default. npm ignores package-lock.json when a shrinkwrap is present, so the root behaviour was the correct one. Covered by new tests at both levels - findNestedLockfiles picked whichever lockfile readdirSync listed first. That matched precedence only because these five names happen to sort that way, and readdirSync guarantees no order. It now picks by precedence explicitly Nested bun.lock now outranks a nested package-lock.json, matching what the root scan has always done. That is a deliberate consequence of the two paths agreeing rather than a separate change. buildNoPackagesMessage listed the supported lockfiles by hand in its error text. It now derives them, so the message cannot go stale. No other behaviour change. src/parsers/index.ts drops from 221 lines to 176 and from 12 ScanInput literals to 3. Verified with the full suite (1830 tests) plus real CLI runs against this repo and fixtures for both precedence cases.
Matches the inline type-import convention already used in nine other files, and marks the notes parameter readonly since the callee never mutates it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
loadPackageshandled the same five lockfiles twice, once for the repo root and once for nested discovery, as two if-chains about 100 lines apart containing 12 near-identicalScanInputliterals.chooseBestLockfileinsrc/utils/file.tsthen kept a third, separate ordering of its own.Three copies of the same knowledge meant three chances to disagree, and they did.
The change
LOCKFILE_PRIORITYinconstants.tsis now the single source of truth for precedence.src/parsers/index.tsholds aLOCKFILE_HANDLERStable keyed by filename saying how to read each one. Root detection, nested discovery andchooseBestLockfileall walk that one list.Keying the table by filename rather than using an array means the compiler requires a handler for every entry in the priority list, so adding a sixth lockfile cannot half-land.
constants.tswas chosen as the home becauseutils/file.tsalready imports from it, which avoids an import cycle.Two precedence bugs this exposed
npm-shrinkwrap.jsonlost topackage-lock.jsonwhen nested. The root chain checked shrinkwrap first, butchooseBestLockfilescoredpackage-lock.jsonat 0 and never scorednpm-shrinkwrap.jsonat all, so it fell through to the default and sorted last. The same repo resolved differently depending on whether the lockfiles sat at the root or one directory down. npm itself ignorespackage-lock.jsonwhen a shrinkwrap is present, so the root behaviour was the correct one. New tests cover both levels.findNestedLockfilespicked whichever lockfilereaddirSynclisted first. That matched precedence only because these five names happen to sort in priority order alphabetically, andreaddirSyncguarantees no ordering. It now picks by precedence explicitly.One deliberate behaviour change
A nested
bun.locknow outranks a nestedpackage-lock.json, which is what the root scan has always done. This follows from the two paths agreeing rather than being a separate decision, but it is a real change for anyone with both files in a subdirectory.Also
buildNoPackagesMessagelisted the supported lockfiles by hand in its error text. It now derives them, so the message cannot go stale.Verification
tsc --noEmitcleanloadPackagesandchooseBestLockfilepackage-lock.jsonand parses 385 packages, and purpose-built fixtures confirm both nested precedence casessrc/parsers/index.tsgoes from 221 lines to 176, and from 12ScanInputliterals to 3.