Auto-delete workspace package-lock.json files#2163
Conversation
|
Is this supposed to only delete the If in general this is a significant change:
So what would be lost without having package lock at all is reliably reproducible builds. Two subsequent builds without repo code change may result in a different result. This is just what needs to be clear and accepted as a risk. |
|
This PR automatically deletes |
lyonsil
left a comment
There was a problem hiding this comment.
This looks like you're applying an update from the multi extension template to paranext-core, but it also looks like you asked superpowers to do it instead of running npm run update-from-templates. Is that going to cause weird conflicts later when we do run npm run update-from-templates?
@lyonsil reviewed 4 files and all commit messages, and made 2 comments.
Reviewable status: 4 of 7 files reviewed, 1 unresolved discussion (waiting on tjcouch-sil).
docs/superpowers/plans/2026-03-30-update-from-templates-package-lock.md line 0 at r1 (raw file):
Do you think we want to commit all the plans and specs that will get generated along the way? My main concern is that the spec+plan files might not reflect what is ultimately checked in, leading to immediately out of date/incorrect specs. But maybe there is value in keeping them around? What do you think?
tjcouch-sil
left a comment
There was a problem hiding this comment.
Yeah, basically I wrote the code here first, then I propagated it over to the templates. I plan to merge this so it's a squash and merge, then I'll update from templates and handle the conflicts after this merges if that's ok.
@tjcouch-sil made 2 comments.
Reviewable status: 2 of 7 files reviewed, 1 unresolved discussion (waiting on lyonsil).
docs/superpowers/plans/2026-03-30-update-from-templates-package-lock.md line at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
Do you think we want to commit all the plans and specs that will get generated along the way? My main concern is that the spec+plan files might not reflect what is ultimately checked in, leading to immediately out of date/incorrect specs. But maybe there is value in keeping them around? What do you think?
Nope, I don't think we need these! Whoops; meant to remove them before I announced the PR. Maybe we should ignore these folders. I dunno. Removed.
lyonsil
left a comment
There was a problem hiding this comment.
@lyonsil reviewed 5 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved.
…in update-from-templates
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ensionFolder Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lates
- Add repoRoot constant and getWorkspaces() cache to avoid re-reading
root package.json on every isUnusedWorkspacePackageLock call
- Extract deleteUnusedPackageLockIfPresent helper to deduplicate the
try/unlink/ENOENT pattern from both formatExtensionsRoot and formatExtensionFolder
- Remove redundant .endsWith('package-lock.json') fast-path from
resolvePackageLockConflicts (isUnusedWorkspacePackageLock already guards this)
- Extract handleConflicts function to deduplicate the 4-branch merge
conflict response pattern shared by multi-template and per-extension catch blocks
- Move formatExtensionsRoot into the final Promise.all alongside
formatExtensionFolder calls — no working changes during subtree work
- Add explanatory comments above all ESLint disable annotations
- Fix stale replaceInFileIgnoreGlobs comment (lock files now deleted proactively)
- Fix lint: eslint-disable for JSON.parse type assertion; replace
NodeJS.ErrnoException with 'code' in error narrowing pattern
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Move minimatch to devDependencies (was incorrectly in dependencies) - Create new #region shared with paranext-extension-template for execAsync/repoRoot - Move cachedWorkspaces/getWorkspaces to after export consts for correct declaration order - Add clarifying comment on non-deterministic Promise.all push order in resolvePackageLockConflicts Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
b9e4454 to
b4bd17a
Compare
dependabot kept spamming us with silly changes to the workspace
package-lock.jsonfiles that are unused. But we wanted to keep these files synced with the template so we didn't have merge conflicts with them when updating from templates. Now, instead of having the useless extensionpackage-lock.jsonfiles in our repo, this PR removes them and automatically resolves merge conflicts with those files by just deleting the file again. Hopefully we don't have to think about those extensionpackage-lock.jsonfiles anymore.Code Review Summary
Branch: dependabot-remove-lock-files
Base: origin/main
Date: 2026-03-31
Review model: Claude Sonnet 4.6
Files changed: 6
Overview
This branch makes
update-from-templatesautomatically deletepackage-lock.jsonfiles in npm workspace subdirectories (underextensions/) so they don't accumulate and cause merge conflicts during template pulls. The approach adds two new exported functions togit.util.ts:isUnusedWorkspacePackageLock(guards that a given path is a workspace-owned lock file) andresolvePackageLockConflicts(called after a failed subtree pull to auto-resolve any conflicts that are solely these unused lock files). Two helper functions —deleteUnusedPackageLockIfPresentandformatExtensionsRoot— handle proactive deletion during the formatting pass. The branch also includes design documents capturing the spec and implementation plan.The logic is well-scoped: only files that pass the
isUnusedWorkspacePackageLockguard are touched automatically, and workspace patterns are read dynamically frompackage.jsonrather than hardcoded. During review,minimatchwas moved fromdependenciestodevDependenciesand a clarifying comment was added to thePromise.allconflict-classification block.API Changes
None. No public API surfaces in
lib/platform-bible-react/,lib/platform-bible-utils/,papi.d.ts, or extension type declaration files were modified.Findings
Critical — Must address before merge
None.
Important — Should address before merge
extensions/package.json:minimatchin wrong dependency section (fixed during review: moved fromdependenciestodevDependencies)extensions/lib/git.util.ts:cwd: repoRootin the shared region referenced a constant defined outside it (fixed during review: extractedexecAsyncandrepoRootinto a new#region shared with paranext-extension-templateblock at the top of the file;cachedWorkspacesandgetWorkspaceswere moved to after the export consts to maintain declaration order.repoRootis now in the shared region and propagation to the template will include it.)[Author response:
minimatchwas a mistake — fixed in-review. For the shared region, an initial attempt to moverepoRootinto the existing shared region was reverted by lint (no-use-before-define) becausegetWorkspacesused it above that region. Fixed properly by creating a new shared region forexecAsync/repoRootat the top of the file and relocatingcachedWorkspaces/getWorkspacesbelow the export consts.]Minor — Consider
git.util.ts~line 231: Non-deterministic push order inPromise.allconflict classifier (fixed during review: added comment explaining that order doesn't matter since all lock files are removed andremainingConflictsis reporting-only)(author removed the explanatory line; not important enough to keep)git.util.tsline.slice(3): No explanation for why rename entries aren't a concernCommit message typo:(not worth fixing — branch will be squash-merged)'exensions root'[Author response: Added clarifying comment for Promise.all ordering. Declined to keep the rename-entry explanation. Typo dismissed — squash merge will clean it up.]
Template Propagation
Shared Regions Modified
extensions/lib/git.util.ts:9–16(new region) —execAsync+repoRootare now marked as shared withparanext-extension-template. Propagate this new region to the template.extensions/lib/git.util.ts:92–(existing region) —execCommandnow usescwd: repoRoot(previously inline). Propagation already needed for this change; covered by the new shared region above. Note:resolvePackageLockConflictsis paranext-core-specific and should NOT be propagated.Extension Config Changes
None.
Positive Observations
resolvePackageLockConflictsis well-scoped: only auto-resolves files that passisUnusedWorkspacePackageLock, leaving all other conflict files untouched — minimal blast radius.git status --porcelainv1 (vs v2 used incheckForWorkingChanges) is explicitly documented with a justification comment — good callout of an intentional divergence.git rmcalls are correctly awaited in afor...ofloop with an explaining comment, avoiding interleaved git index updates.getWorkspacescaches its result — avoids repeated filesystem reads across the loop.handleConflicts(resolved-but-others-remain, not-resolved-with-conflicts, not-a-conflict-error) gives clear diagnostics for each failure mode.isUnusedWorkspacePackageLockhas layered defensive checks (filename, directory prefix, workspace membership) before touching anything.package.jsonrather than hardcoded — correct and future-proof.deleteUnusedPackageLockIfPresentcorrectly swallows onlyENOENTand re-throws everything else.Interview Notes
Author's stated purpose: auto-delete unused workspace
package-lock.jsonfiles duringupdate-from-templatesso merge conflicts with these files don't waste time during template pulls.minimatchbeing independencieswas a mistake — fixed in-review. The shared regionrepoRootissue was acknowledged; author plans to propagate manually toparanext-extension-templatelater. Author understands the tradeoff: fully fixing the shared-region issue in-repo would require movinggetWorkspacesinto the shared region. No tests added for the new helper functions — author explicitly decided against it for these build tooling scripts.In-Review Quality Check
Changes made during review:
extensions/package.json:minimatchmoved todevDependenciesextensions/lib/git.util.ts: clarifying comment added toPromise.allblockextensions/lib/git.util.ts: new#region sharedblock created forexecAsync/repoRoot;cachedWorkspaces/getWorkspacesmoved to after export consts (resolves the shared-region constant issue cleanly)Results: typecheck passed, lint passed, format passed, tests all passed.
Suggested Review Focus
paranext-extension-template: The newexecAsync/repoRootshared region (lines 9–16) needs to be propagated to the template, along with thecwd: repoRootchange inexecCommand. Author plans to do this as a follow-up — confirm it's tracked.resolvePackageLockConflictsplacement: This function is paranext-core-specific (depends on workspace structure). Confirm it should not be in the shared region.The following is AI-generated slop about the PR. I cut out some stuff that was even more of a waste of time to read. You can ignore it if you want:
Changes
isUnusedWorkspacePackageLock— checks whether a repo-root-relative path is an unused workspace lock file by matching its parent directory against the workspace patterns in the rootpackage.jsonresolvePackageLockConflicts— after a failedgit subtree pull, scansgit status --porcelainfor conflict entries, auto-resolves any unused workspacepackage-lock.jsonconflicts viagit rm, and returns the remaining conflicts (if any)update-from-templates— wraps both the multi-extension and per-extension subtree pulls in conflict-resolution logic; commits automatically when all conflicts are lock files, or surfaces a clean error when real conflicts remainformatExtensionFolder/formatExtensionsRoot— deletes unused lock files during the post-merge formatting pass (handles lock files introduced by clean merges, not just conflicted ones)repoRootconstant,getWorkspaces()cache,deleteUnusedPackageLockIfPresenthelper,handleConflictsfunction extracted to eliminate duplication; explanatory comments added above all ESLint disablesTesting
npm run update-from-templatesend-to-end; auto-resolved 1 multi-template conflict (DU), 1 per-extension conflict (UU), and cleaned up 8 more viaformatExtensionFolderThis change is