[#3060] Synchronised npm lock files when the installer removes front-end tools. - #3062
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 18 minutes Limit details: You’ve used all 2 included reviews currently available. Your 82 included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 31 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (27)
📒 Files selected for processing (6)
Comment |
|
Code coverage (threshold: 90%) Per-class coverage |
This comment has been minimized.
This comment has been minimized.
2 similar comments
|
Code coverage (threshold: 90%) Per-class coverage |
|
Code coverage (threshold: 90%) Per-class coverage |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3062 +/- ##
==========================================
- Coverage 87.08% 86.95% -0.14%
==========================================
Files 106 100 -6
Lines 4994 4928 -66
Branches 58 3 -55
==========================================
- Hits 4349 4285 -64
+ Misses 645 643 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
📖 Documentation preview for this pull request has been deployed to Netlify: https://6a87acc7163f70cb205e504a--vortex-docs.netlify.app This preview is rebuilt on every commit and is not the production documentation site. |
AlexSkrypnyk
added
the
Needs review
1 participant
Closes #3060
Problem
When the installer removed a front-end tool it rewrote
package.jsonviaJsonManipulator::updateFile()but leftpackage-lock.jsonuntouched.ahoy feirunsnpm ci, which treats the lock as authoritative and aborts when the pair disagrees, so a generated project failed on its first build. Full deselection was safe because thefrontend_allgroup removes both files together; partial deselection (ESLint off, Jest kept) was the broken path.Fix
A new
DrevOps\VortexInstaller\Utils\NpmLockutility reconciles the lock file with the manifest next to it.NpmLock::sync()copies the manifest's dependency blocks (dependencies,devDependencies,optionalDependencies,peerDependencies) onto the lock's rootpackages[""]entry, then garbage-collects everypackagesentry no longer reachable from a root, following node's resolution rules: the innermostnode_modulescopy wins, falling back outwards to the hoisted tree. It is offline and deterministic, with no npm invocation, because Node is not an installer prerequisite. It writes the lock the way npm does: two-space indent, unescaped slashes, trailing newline.It is wired in at the manifest edit itself, in the new
Tools::updateNpmManifest(), so the sync is derived from the tool map's single list of package names rather than a second list that could drift out of sync. Every tool in the map gets it, not just the one that exposed the bug.Custom theme treatment
The custom theme carries its own
package.jsonandpackage-lock.json, with the full ESLint, Prettier and Stylelint dependency sets and its own lint scripts, and the tool map never touched them, so a project that deselected ESLint still installed and ran ESLint inside the theme, andahoy lint-fecalls the theme'snpm run lintdirectly. Newtheme.package.jsonentries on the ESLint and Stylelint tool definitions clean the theme manifest, and its lock is synced the same way as the root pair. The theme manifest and lock are never deleted, even when every front-end tool is deselected, because they also drive the Sass/PostCSS/Terser build.Dead aggregate scripts
With both linters deselected, each linter's callback rewrote
lintto call the other one, leavinglintandlint-fixpointing at scripts that no longer exist. Thefrontend_lintinggroup now removes that pair from both manifests, drops the theme lint lines from.ahoy.yml, and drops the two matching rows from the themeREADME.mdcommand table.CI configs
The theme lint step in
.github/workflows/build-test-deploy.ymland.circleci/config.ymlwas fenced only onDRUPAL_THEME, so with both linters deselected CI still rannpm run lintin the theme against a script that had been removed. Both steps now sit inside theTOOL_ESLINT_STYLELINTfence as well.Refactor
The content-removal pass (
stringsandlines) was extracted fromprocessTool()intoprocessContent()and is now shared withprocessGroup(), so groups can declare content removals the same way tools do. It is skipped when a definition declares neither key.Tests
New
NpmLockTestunit suite (18 cases) covers root reconciliation, orphan pruning, retention of transitives shared with a surviving dependency, nestednode_modulesshadowing and fall-back to the hoisted tree, workspace/link entries as roots, unresolvable optional dependencies, a missing lock, an unsupported lock, non-object dependency blocks, npm-shaped output formatting and empty-object round-tripping.ToolsHandlerProcessTestgains manifest/lock sync assertions on the partial-deselection scenarios plus a newtools_no_eslint_no_stylelintfixture scenario (both linters off, Jest kept). Snapshots regenerated.Fixture snapshots continue to ignore lock file content via
.ignorecontent, so the lock is asserted programmatically in the tests rather than as an 8,600-line fixture diff.Verification
The PHP prune was checked against real npm 10.9.2: on the template's own root pair it produced exactly the same 421-entry package set as
npm install --package-lock-only, with no differences beyondlibcmetadata that npm drops and this preserves. Running the installer end-to-end with both linters deselected then produced a project wherenpm ci --dry-runsucceeds at the root (331 packages) and in the theme (175 packages).Screenshots
N/A
Before / After