fix(flip-ui): correct dependency scoping, stop shipping Mirage, and enforce both in CI (#1041) - #1061
fix(flip-ui): correct dependency scoping, stop shipping Mirage, and enforce both in CI (#1041)#1061atriaybagur wants to merge 7 commits into
Conversation
Dependabot derives an alert's "Development" scope label from whether a package is reachable only through devDependencies. Sixteen packages that `src/` imports — and that vite therefore bundles into the production `dist/` — were declared as devDependencies, so an alert on any of them would arrive labelled "Development" and read as not-shipped, when it is in the bundle deploy-ui syncs to S3 and CloudFront serves. `husky`, a git-hook tool, had drifted the other way. Move all sixteen into dependencies and husky into devDependencies. Version specifiers are unchanged; the regenerated lockfile carries no version/resolved/integrity change, only the `dev: true` flags that follow from the stanza move. The change is inert at build and install time: vite tree-shakes from the `src/` entry points regardless of stanza, and no install path in the repo uses --omit=dev. Verified by building `dist/` from develop's manifest and from this one — all 113 files match by content-hashed name and byte size. #1041 originally listed eight packages. A re-sweep found sixteen; the issue body has been corrected. The eight it missed are @headlessui/vue, @popperjs/core, codemirror-editor-vue3, date-fns, mime, mitt, underscore and vuejs-smart-table (the last imported by src/main.ts itself). highlight.js is deliberately left in dependencies. It has no static import, so a `from "..."` grep reads it as dev-only, but it is loaded via `import("highlight.js/lib/core")` in src/utils/highlightJson.ts and does ship — hence the README note documents matching dynamic import() too, so the next audit does not "fix" it in the wrong direction. Signed-off-by: at24_bioeng625-pc <alexandre.triay_bagur@kcl.ac.uk>
|
✅ Acceptance criteria have been automatically imported from the linked issue(s) and added to the PR description. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…1041) Review of the first commit found that its own stated rule — "if a non-test file under src/ imports it, it is a dependency" — was under-inclusive, and that the tree violated it. src/main.ts statically imported ../mocks/server, which is outside src/ and therefore outside the sweep. Three things follow, and all three are fixed here. 1. Mirage was shipping in the production bundle. The VITE_LOCAL branch that calls makeServer() folds correctly, but miragejs patches Error.prototype at module scope, so rolldown kept the module and emitted ~230 of them — Mirage, Pretender, route-recognizer, inflected and all of lodash — into the entry chunk of every build. This is the exact FLIP#794 mechanism that src/main.ts already documents for mocks/demo-server; that one was fixed with a dynamic import, this one never was. Making it dynamic cuts the entry chunk from 331 KB to 182 KB and total shipped JS by 128 KB. 2. Four more packages were mis-scoped. miragejs and uuid ship in the public /ark_demo bundle via mocks/demo-server, so they are dependencies whatever happens to the main bundle. codemirror and tippy.js are imported directly by src/ but were declared in neither stanza — they resolved only through npm hoisting a parent's tree, so a dedupe or a parent bump would have broken the build, and Dependabot could not scope-label them at all. 3. Prose was never going to hold this line. eslint.config.mjs now runs import-x/no-extraneous-dependencies over src/ (minus tests) and mocks/, so a devDependency imported from production source fails `npm run lint` in CI at the moment it is written. mocks/ is deliberately not exempt — it ships in the demo bundle. Verified adversarially: demoting axios back to devDependencies reddens the lint. Bringing mocks/ under the linter surfaced 19 pre-existing errors there (it was never in the lint glob). Sixteen were formatting, auto-fixed. The rest were one stale eslint-disable naming @typescript-eslint/ban-types, a rule removed in typescript-eslint v8 and split into no-empty-object-type — so it had been suppressing nothing; it now names the rule that exists. @popperjs/core stays in dependencies even though tree-shaking currently drops it, because its only consumers (AiSelect/AiChipSelect) are unused. Scoping keys off the import graph, not the bundle: what production code imports is stable, whereas what survives tree-shaking flips whenever an unrelated component starts or stops being used. Totals: 21 packages corrected — 18 out of devDependencies, 2 that were declared nowhere, and husky the other way. Signed-off-by: at24_bioeng625-pc <alexandre.triay_bagur@kcl.ac.uk>
|
✅ Acceptance criteria have been automatically imported from the linked issue(s) and added to the PR description. |
|
Addressed in 5a4cb89 — the Mirage removal is now enforced on both sides: a |
| "vue-router": "^4.0.11" | ||
| "vue-router": "^4.0.11", | ||
| "vue-tippy": "^6.3.1", | ||
| "vuejs-smart-table": "^1.0.0-beta.5", |
There was a problem hiding this comment.
vuejs-smart-table is added here as a new production dependency, but I can't find any import, plugin registration (app.use(...)), or template reference to it anywhere in flip-ui/src or flip-ui/mocks on this branch. The lockfile also gains a brand-new node_modules/vuejs-smart-table node (with its own new vue-demi transitive) rather than a version bump of something pre-existing -- so this isn't just a stanza move like the other packages here. Since this PR's whole premise is "scope keys off the import graph," an entry with no import at all seems to contradict that on its own terms.
| until it matters. What it changes is the **scope label on a security alert**: a package that ships to users | ||
| but sits in `devDependencies` produces an alert labelled "Development", which reads as *not in the production | ||
| bundle* and invites a wrongly-dismissed alert on code CloudFront is serving. FLIP#1041 corrected 21 | ||
| packages: 18 moved out of `devDependencies`, two that were declared nowhere at all (`codemirror`, |
There was a problem hiding this comment.
This states "18 moved out of devDependencies, two that were declared nowhere at all (codemirror, tippy.js)". Based on the package.json/lockfile diff, vuejs-smart-table was also declared in neither stanza before this PR (no prior dependencies/devDependencies entry, no prior lockfile node) -- so it looks like the "declared nowhere" bucket should be 3, not 2, and the "moved from devDependencies" count should be adjusted down accordingly.
garciadias
left a comment
There was a problem hiding this comment.
The mechanics of the fix (stanza corrections, the Mirage dynamic-import fix, the two new ESLint guards, and the artefact-sentinel tests) are solid and CI is fully green, but vuejs-smart-table is added as a new production dependency with no import anywhere in the codebase -- directly contradicting both the PR's stated "import graph" methodology and an explicit claim in its first commit message that it's imported by src/main.ts (it is not). Since accurate dependency attribution is this PR's entire reason for existing, please confirm whether this package is actually needed yet, and correct the README's package count accordingly.
Two conflicts, both in flip-ui's manifest and lockfile, where #1042 (Cypress 14.5.2 -> 15.21.0) landed on develop against this branch's dependency re-scoping. - package.json: kept this branch's stanza layout (the 21 re-scoped packages, @popperjs/core gone via #1065, eslint-plugin-import-x, the lint glob covering mocks/) and took develop's versions and scripts on top: cypress pinned 15.21.0, cypress-localstorage-commands ^2.3.0, the test:types script, the cypress/included:15.21.0 image, and the removal of @cypress/vite-dev-server and @cypress/vue. - package-lock.json: not merged by hand. Started from develop's lockfile and re-resolved it against the merged manifest with `npm install --package-lock-only`, so the only differences from develop are this branch's own: the root stanza move, the dev-flag flips, and the 9 new entries of the import-x plugin tree. No pre-existing package changed version, resolved or integrity; extract-zip stays gone. Verified on the merged tree: npm ci clean, lint 0 errors, test:types clean, 1307 unit tests passing, build:deploy's assert-no-demo-artefacts guard green. Signed-off-by: at24_bioeng625-pc <alexandre.triay_bagur@kcl.ac.uk>
Closes #1041.
Why
Dependabot derives a security alert's scope label from which stanza a package occupies. Packages that
flip-uiships to users were sitting indevDependencies, so an alert onaxios,piniaoraws-amplifywould arrive labelled "Development" — reading as not in the production bundle when it is in the bundlemake deploy-uiserves through CloudFront. In the #1036 sweep the label happened to hold for all three alerts, but that was luck.What shipped
21 packages corrected: 18 moved out of
devDependencies, 2 that were declared in neither stanza (codemirror,tippy.js— they resolved only through npm hoisting a parent's tree), andhusky, which had drifted the other way.Mirage is no longer in the production bundle.
src/main.tsstatically imported../mocks/server. TheVITE_LOCALbranch that callsmakeServer()folds correctly, but miragejs patchesError.prototypeat module scope, so rolldown kept the module and emitted ~230 of them — Mirage, Pretender, route-recognizer, inflected and all of lodash — into the entry chunk of every build. This is the exact FLIP#794 mechanismsrc/main.tsalready documents formocks/demo-server; that one was fixed with a dynamic import, this one never was.The rule is now enforced, not just documented.
eslint.config.mjsrunsimport-x/no-extraneous-dependenciesoversrc/(minus tests) andmocks/, so a devDependency imported from production source failsnpm run lintin CI at the moment it is written.mocks/is deliberately not exempt —mocks/demo-server.tsships in the public/ark_demobundle, which is whymiragejsanduuidare dependencies regardless of the main-bundle fix.The Mirage removal itself is enforced too (review follow-up):
miragejsis a legitimatedependency— the demo bundle ships it — sono-extraneous-dependenciescannot flag a static re-import ofmocks/server. Aflip-ui/no-static-mirageno-restricted-importsblock now rejects static imports ofmiragejs/pretender/either mock server fromsrc/production sources (the rule does not match dynamicimport(), so the legalbootstrap()loads pass), andassert-no-demo-artefactscarries Mirage/Pretender library-code sentinels so the artefact check also catches the class no source lint can see — a folded mock branch that stops folding.Plus the Dependency scoping section in
flip-ui/README.md(AC3).Notes for the reviewer
package.jsonandeslint.config.mjs; skim the lock. The lockfile diff is large but contains zero changes to anyversion/resolved/integrityof a pre-existing package — only"dev": trueflag flips, the root stanza move, and the 7 packages of the new lint plugin's tree.@popperjs/coreis independencieseven though tree-shaking currently drops it (its only consumers,AiSelect/AiChipSelect, are unused components). Scoping keys off the import graph, not the bundle: what production code imports is stable, whereas what survives tree-shaking flips whenever an unrelated component starts or stops being used. Over-declaring is the safe direction — it labels an alert "Production" for something that might ship.highlight.jsis deliberately untouched. It has no static import — afrom "..."grep reads it as dev-only — but it loads viaimport("highlight.js/lib/core")and does ship. It was already correct.mocks/are fixed here, surfaced only becausemocks/entered the lint glob for the first time. 16 were formatting (auto-fixed); the rest stemmed from a staleeslint-disablenaming@typescript-eslint/ban-types, a rule removed in typescript-eslint v8 and split intono-empty-object-type— so it had been suppressing nothing.eslint-plugin-import-xrather thaneslint-plugin-import: the latter caps at ESLint^9and this repo is on^10.1.0. Published 2026-06-28, well clear of the 72-hour cooldown.Verification
npm cinpm cihard-fails on stanza driftnpm run lintsrc/andmocks/(3 pre-existingmax-lenwarnings in an untouched file)axiosback todevDependenciesreddens the lint with 3 errorsmiragejs/../mocks/server/../mocks/demo-serverreddens the lint with 3no-restricted-importserrors; a realbuild:deploypasses with all 7 sentinels absent from 93 dist filesnpm run test:unitnpm run build:deploycheck-build-flags+assert-no-demo-artefactspassmiragejs/pretender/mocks/matches in the production bundledemo-serverchunk, out of the demo entry chunk tooThe first commit was verified separately to produce a byte-identical bundle (all 113 files matching by content-hashed name and size), isolating the pure-relabelling change from the Mirage fix in the second.
developis merged back in fc91312 — the conflict waspackage.json/package-lock.jsononly, resolved keepingvue-tippy/yupindependenciesper this PR; the lockfile was re-resolved withnpm installagainst the merged manifest. Merged back again in 0d5e5f9 after #1042 (Cypress 15.21.0) landed: the same two files conflicted;package.jsonkeeps this PR's stanza layout with develop's versions and scripts on top (cypress15.21.0,cypress-localstorage-commands^2.3.0,test:types, the@cypress/*component-testing packages gone), and the lockfile was again re-resolved from develop's copy against the merged manifest — zero version/resolved/integrity changes to pre-existing packages,extract-zipstill absent.Acceptance Criteria
Imported from issue #1041
devDependenciestodependencies, andhuskymoves the other way.npm ci && npm run build:deploystill produces a working bundle, and the flip-ui unit + Cypress suites pass.flip-ui/README.mdstating the rule — if a non-test file undersrc/imports it, statically or via dynamicimport(), it is adependency— so the drift does not recur.