Skip to content

Auto-delete workspace package-lock.json files#2163

Open
tjcouch-sil wants to merge 12 commits intomainfrom
dependabot-remove-lock-files
Open

Auto-delete workspace package-lock.json files#2163
tjcouch-sil wants to merge 12 commits intomainfrom
dependabot-remove-lock-files

Conversation

@tjcouch-sil
Copy link
Copy Markdown
Member

@tjcouch-sil tjcouch-sil commented Mar 31, 2026

dependabot kept spamming us with silly changes to the workspace package-lock.json files 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 extension package-lock.json files 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 extension package-lock.json files 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-templates automatically delete package-lock.json files in npm workspace subdirectories (under extensions/) so they don't accumulate and cause merge conflicts during template pulls. The approach adds two new exported functions to git.util.ts: isUnusedWorkspacePackageLock (guards that a given path is a workspace-owned lock file) and resolvePackageLockConflicts (called after a failed subtree pull to auto-resolve any conflicts that are solely these unused lock files). Two helper functions — deleteUnusedPackageLockIfPresent and formatExtensionsRoot — 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 isUnusedWorkspacePackageLock guard are touched automatically, and workspace patterns are read dynamically from package.json rather than hardcoded. During review, minimatch was moved from dependencies to devDependencies and a clarifying comment was added to the Promise.all conflict-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: minimatch in wrong dependency section (fixed during review: moved from dependencies to devDependencies)

  • extensions/lib/git.util.ts: cwd: repoRoot in the shared region referenced a constant defined outside it (fixed during review: extracted execAsync and repoRoot into a new #region shared with paranext-extension-template block at the top of the file; cachedWorkspaces and getWorkspaces were moved to after the export consts to maintain declaration order. repoRoot is now in the shared region and propagation to the template will include it.)

[Author response: minimatch was a mistake — fixed in-review. For the shared region, an initial attempt to move repoRoot into the existing shared region was reverted by lint (no-use-before-define) because getWorkspaces used it above that region. Fixed properly by creating a new shared region for execAsync/repoRoot at the top of the file and relocating cachedWorkspaces/getWorkspaces below the export consts.]

Minor — Consider

  • git.util.ts ~line 231: Non-deterministic push order in Promise.all conflict classifier (fixed during review: added comment explaining that order doesn't matter since all lock files are removed and remainingConflicts is reporting-only)

  • git.util.ts line.slice(3): No explanation for why rename entries aren't a concern (author removed the explanatory line; not important enough to keep)

  • Commit message typo: 'exensions root' (not worth fixing — branch will be squash-merged)

[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 + repoRoot are now marked as shared with paranext-extension-template. Propagate this new region to the template.
  • extensions/lib/git.util.ts:92– (existing region) — execCommand now uses cwd: repoRoot (previously inline). Propagation already needed for this change; covered by the new shared region above. Note: resolvePackageLockConflicts is paranext-core-specific and should NOT be propagated.

Extension Config Changes

None.

Positive Observations

  • resolvePackageLockConflicts is well-scoped: only auto-resolves files that pass isUnusedWorkspacePackageLock, leaving all other conflict files untouched — minimal blast radius.
  • git status --porcelain v1 (vs v2 used in checkForWorkingChanges) is explicitly documented with a justification comment — good callout of an intentional divergence.
  • Sequential git rm calls are correctly awaited in a for...of loop with an explaining comment, avoiding interleaved git index updates.
  • getWorkspaces caches its result — avoids repeated filesystem reads across the loop.
  • Three-way error logging in handleConflicts (resolved-but-others-remain, not-resolved-with-conflicts, not-a-conflict-error) gives clear diagnostics for each failure mode.
  • isUnusedWorkspacePackageLock has layered defensive checks (filename, directory prefix, workspace membership) before touching anything.
  • Workspace patterns are read dynamically from package.json rather than hardcoded — correct and future-proof.
  • deleteUnusedPackageLockIfPresent correctly swallows only ENOENT and re-throws everything else.

Interview Notes

Author's stated purpose: auto-delete unused workspace package-lock.json files during update-from-templates so merge conflicts with these files don't waste time during template pulls.

minimatch being in dependencies was a mistake — fixed in-review. The shared region repoRoot issue was acknowledged; author plans to propagate manually to paranext-extension-template later. Author understands the tradeoff: fully fixing the shared-region issue in-repo would require moving getWorkspaces into 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: minimatch moved to devDependencies
  • extensions/lib/git.util.ts: clarifying comment added to Promise.all block
  • extensions/lib/git.util.ts: new #region shared block created for execAsync/repoRoot; cachedWorkspaces/getWorkspaces moved to after export consts (resolves the shared-region constant issue cleanly)

Results: typecheck passed, lint passed, format passed, tests all passed.

Suggested Review Focus

  • Shared region propagation to paranext-extension-template: The new execAsync/repoRoot shared region (lines 9–16) needs to be propagated to the template, along with the cwd: repoRoot change in execCommand. Author plans to do this as a follow-up — confirm it's tracked.
  • resolvePackageLockConflicts placement: 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 root package.json
  • resolvePackageLockConflicts — after a failed git subtree pull, scans git status --porcelain for conflict entries, auto-resolves any unused workspace package-lock.json conflicts via git 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 remain
  • formatExtensionFolder / formatExtensionsRoot — deletes unused lock files during the post-merge formatting pass (handles lock files introduced by clean merges, not just conflicted ones)
  • RefactoringrepoRoot constant, getWorkspaces() cache, deleteUnusedPackageLockIfPresent helper, handleConflicts function extracted to eliminate duplication; explanatory comments added above all ESLint disables

Testing

  • Integration test: ran npm run update-from-templates end-to-end; auto-resolved 1 multi-template conflict (DU), 1 per-extension conflict (UU), and cleaned up 8 more via formatExtensionFolder
  • TypeScript tests pass
  • Lint passes

Open with Devin

This change is Reviewable

tjcouch-sil and others added 11 commits March 30, 2026 15:09
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>
@tjcouch-sil tjcouch-sil changed the base branch from ai/main to main March 31, 2026 15:29
@tjcouch-sil tjcouch-sil changed the title Auto-resolve package-lock.json merge conflicts in update-from-templates Auto-delete package-lock.json during merge conflicts and during formatting in update-from-templates Mar 31, 2026
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 9 additional findings.

Open in Devin Review

- 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>
@tjcouch-sil tjcouch-sil changed the title Auto-delete package-lock.json during merge conflicts and during formatting in update-from-templates Auto-delete workspace package-lock.json files Mar 31, 2026
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.

1 participant