Fix a red-on-next-run CI gate, a blind CI gate, and adapter tree-shaking - #33
Merged
Merged
Conversation
The `npm audit --omit=dev --audit-level=high` gate in quality-gates.yml started failing on GHSA-2v37-7h3g-55p8 (nanoid <3.3.18) without any code change on develop. nanoid reaches the tree only through vue -> @vue/compiler-sfc -> postcss, which is build-time only. Two things were wrong: - The root is private and publishes nothing, so react, react-dom and vue belong in devDependencies. They exist purely to resolve the packages' peers while building and testing, and keeping them in `dependencies` is what put vue's build-time tree in front of an `--omit=dev` audit. - nanoid was left vulnerable in the toolchain we actually run. Pin it forward with an override so it is patched, not merely excluded from the audit's scope. All four packages declare `dependencies: {}`, and no workflow installs with --omit=dev or --production, so nothing else changes resolution. Lockfile regenerated with npm@10, not the locally installed npm 11 which prunes other platforms' optional binaries: entry count is unchanged at 1606 with no additions or removals, and all 100 linux entries intact.
show-sizes.js guessed formats from file extensions, assuming index.js is CJS and index.mjs is ESM. That holds for core and react but inverts for vue, which is `"type": "module"` and so ships ESM as dist/index.js and CommonJS as dist/index.cjs. The report therefore printed vue's ESM bundle under a CJS label and never printed its real CJS bundle at all. Angular was missing entirely, having never been added to the list. Resolve each entry from the package's own manifest instead (exports['.'].import/require, falling back to module/main), which also handles ng-packagr's ESM-only output where main and module point at the same fesm2022 bundle -- reporting that file twice would invent a CJS build that is not shipped. Before After vue (CJS): 36.31 KB vue (ESM): 36.31 KB (no vue ESM) vue (CJS): 39.37 KB (no angular) angular (ESM): 54.47 KB The collector is now exported and covered by scripts/show-sizes.test.js, which builds throwaway workspaces on a real filesystem rather than mocking fs, plus two regression tests against the repo's own packages. Root vitest.config.mjs scopes that suite to scripts/** so it cannot touch the per-package coverage floors, and quality-gates.yml runs it after the build step, where a dist exists for those two tests to read.
Running the angular suite locally leaves an orphaned packages/angular/vitest.config.ts.timestamp-<n>-<hash>.mjs behind when vitest does not get to clean it up, which on Windows is often. The file was ignored by neither .gitignore nor .prettierignore, so it showed up as untracked and, because `prettier --check .` walks untracked files, turned the next format-check red for reasons unrelated to any change. CI never hit this because format-check runs before any test, so the artifact only ever bit local runs and risked being committed by accident.
packages/vue peer-depends on vue but never declared it in devDependencies, so its build, typecheck and test runs resolved vue only because the workspace root happened to hoist one. packages/react and packages/angular already dev-install their own peers; vue was the outlier, and it became load-bearing on the root's declaration right as that root entry moved to devDependencies. Extend verify-framework-deps.js, which already gates this class of manifest drift in CI, with the rule that every peer must also appear in devDependencies. The script now exports its check so it can be tested, and scripts/verify-framework-deps.test.js covers both rules against throwaway workspaces plus the repo's real manifests. Running the new rule against this repo reported exactly one violation, which is the one fixed here: vue: peer-depends on vue but does not declare it in devDependencies, so it builds and tests against whatever the workspace root happens to hoist Lockfile regenerated with npm@10: 1606 entries unchanged with nothing added or removed, all 100 linux entries intact. The diff is `"dev": true` markers appearing on vue's transitive tree, which is now dev-only everywhere it is reachable. No changeset: devDependencies are not installed by consumers, so no published package changes behaviour or its dependency contract.
isSourceFile() tested paths against /\.(ts|tsx|js|jsx)$/. In a regex
literal `\` matches a literal backslash, not a dot, so the pattern
required a backslash followed by any character and then `ts`/`tsx`/etc
at end of string. No path in the repo can satisfy that, so the predicate
returned false for every file, the walk callback returned early every
time, and `violations` was always empty.
The gate has therefore been reporting "OK: No cross-framework imports
found" unconditionally since it was written -- it would have passed a
package importing another framework wholesale. Verified by appending
`import { DynamicInput } from '@dynamic-field-kit/react'` to
packages/vue/src/index.ts: before this change the script printed OK and
exited 0; after it, it exits 1 and names the file and line.
Fix the escape, and restructure the script the way show-sizes.js and
verify-framework-deps.js already are: export the check, guard the CLI
behind require.main so importing it does not call process.exit, and
derive the scanned roots from a package list rather than three
hand-written paths. scripts/check-cross-framework-imports.test.js covers
import and require forms, nested directories, line numbers, that core
stays allowed, and that non-source files are skipped.
Running the now-working check against this repo finds no violations, so
nothing in packages/*/src had drifted while the gate was blind.
`npm run lint` only covered packages/*/src, so nothing under scripts/ was ever linted in CI. The pre-commit hook does lint it, because lint-staged matches *.js anywhere, which meant the two gates disagreed: a script could sit in the repo with errors that would block the next person who happened to touch it. Add scripts to the lint script and clear the errors that surfaced: - build-changed.js: import order - check-cross-framework-imports.js: prefer-const, curly (already rewritten in the previous commit) - diagnose-hoist.js: execSync's return value was assigned to an unused binding. It runs with stdio: 'inherit', so output goes straight to our stdout and there is nothing to capture; drop the assignment. All three are behaviour-preserving.
With packages/vue dev-installing vue, the root's react, react-dom and vue entries are redundant: every workspace that needs them declares them (packages/react, packages/vue, smoke). Leaving them meant the root's manifest implied ownership of dependencies it does not use, and a package could silently go back to borrowing from the root without verify-framework-deps.js noticing, since resolution would still succeed. @types/react moves to packages/react rather than being dropped. It is genuinely needed -- packages/react is a .tsx package and its `tsc -p tsconfig.json --noEmit` gate resolves React's types through it -- but it was only ever declared at the root, so that gate depended on hoisting too. Verified by running typecheck after the move. Root devDependencies are now exactly the workspace-wide tooling: changesets, commitlint, eslint and its plugins, husky, lint-staged, prettier, tsup, typescript, vitest. Lockfile regenerated with npm@10: 1606 entries unchanged with nothing added or removed, all 100 linux entries intact.
Both demos shipped their scaffold defaults to the live site. The React demo -- the one the package READMEs link to most -- rendered a browser tab and link preview reading "Create Next App", described as "Generated by create next app". The Vue demo's title was "vue-app". Only the Angular demo had a real one. Match Angular's existing "Dynamic Field Kit - <Framework> Example" and give each a description naming what the demo actually shows. Verified in the built output: all three now emit their own title and no stale create-next-app description survives.
Cross-checking every symbol the four READMEs tell you to import against the built .d.ts found no stale API -- all 58 resolve -- but 25 public exports were never mentioned anywhere. All four packages now describe their full surface. The substantive gap was async validation. `validateField` and `validateFields` are synchronous and cannot await, so a `validate` hook returning a Promise is discarded and the field reads as valid. Both `useDynamicForm` and `createDynamicFormStore` use the sync path throughout, including in handleSubmit, so async rules never surface unless the app calls `validateFieldsAsync` itself. Core's adapter section already said async schemas "must be validated through validateFieldsAsync", but no README showed how, and the async functions appeared in no adapter export list at all. Core now has a "Sync vs async validation" section, and each adapter repeats the caveat and links to it. Also documented: - core: `Properties`, `ValidatorFn`, `FieldValidatorResult` and `FieldValidatorFunction`, which appear in signatures throughout the README but had no definition; and `FormStep` next to `WizardState`. - react/vue/angular: `validateFieldAsync`, `validateFieldsAsync`, `resolveOptions` and `validators`, grouped under a heading that marks them as core re-exports rather than adapter exports. - angular: `layoutRegistry`, `LayoutRegistry`, `ColumnLayout`, `RowLayout`, `GridLayout` and `BaseInputComponent`. It was the only adapter documenting no layout registry, and its registry is the one real difference between the three -- it holds standalone components, not render functions -- so it needed its own example rather than a pointer at the React one. Demo links: the package READMEs each pointed only at their framework's landing page. React now links its enterprise-features and wizard routes, core links all three framework demos plus the wizard it documents, and vue/angular note that their wizard is a tab rather than a separate URL. All links verified to serve real pages. Angular's `## What it exports` becomes `## Exports`, matching react/vue.
Components were declared as bare top-level calls -- defineComponent({...})
in vue, React.memo(...) in react. A bundler cannot prove such a call is
side-effect free, so it evaluates it even when the result is unused. That
pinned every default renderer and every component into an app's bundle
regardless of how little of the package it imported.
Annotating those calls /* @__PURE__ */ makes them droppable. Measured
with esbuild, minified, framework external:
react, one core helper 11,149 -> 3,258 B (-71%)
react, DynamicInput only 11,150 -> 6,830 B (-39%)
react, everything 17,601 -> unchanged
vue, one core helper 17,379 -> 13,934 B (-20%)
vue, DynamicInput only 17,379 -> 13,934 B (-20%)
vue, everything 20,530 -> unchanged
"Everything" staying flat is the expected result: nothing is droppable
when it is all reachable. react gains more than vue because its default
renderers are already plain arrow functions, so only the two memo() calls
were holding them; vue's renderers are each a defineComponent() call.
The published dist grows marginally (react +0.03 KB, vue +0.29 KB) since
the annotations are comments carried into the bundle. That is the trade:
a slightly bigger file on npm for a materially smaller bundle in the app.
Also declare "sideEffects": false on core. It has no top-level execution
-- the only module-scope work is `new FieldRegistry()` assigned to an
export -- so the claim is accurate. It buys nothing under esbuild, which
already tree-shakes core to 907 B for a single import, but it is correct
metadata for bundlers that trust the flag over their own analysis. The
adapters cannot claim it: their entry side-effect-imports the default
layouts to register them.
No behaviour change -- these are comments and a packaging flag. Full
suite green: core/react/vue/angular tests, smoke, typecheck, type tests,
lint, and the three verify scripts.
MultiFieldInput renders itself recursively for repeatable groups and
reached itself through a module-scope assignment:
let multiFieldInputSelfRef: Component;
const MultiFieldInput = defineComponent({ ... });
multiFieldInputSelfRef = MultiFieldInput;
That last line is a bare top-level assignment -- a side effect no
bundler is allowed to drop. It anchored MultiFieldInput -> FieldInput ->
DynamicInput -> getDefaultRenderer -> every default renderer into any
app that imported the package at all, which is why the previous commit's
/* @__PURE__ */ annotations only recovered 3.4 KB of a 17 KB floor: the
components were pure but still reachable.
Replace it with a hoisted function declaration returning MultiFieldInput.
A function body is not evaluated until called, so nothing is retained
until a group actually renders. The explicit `Component` return type does
the job the forward-declared `let` was there for -- it stops TypeScript
having to infer MultiFieldInput from inside its own initializer.
Measured with esbuild, minified, vue external:
one core helper 13,934 -> 2,479 B (-82%)
DynamicInput only 13,934 -> 9,196 B (-34%)
MultiFieldInput only 13,944 -> 13,958 B (+14 B)
everything 20,530 -> 20,543 B (+13 B)
Apps using the whole surface pay 13-14 bytes for the wrapper. Against the
original baseline, before the PURE annotations, a vue app importing only
DynamicInput goes 17,379 -> 9,196 B, a 47% reduction.
react and angular were checked for the same pattern and have none.
No behaviour or type change: dist/index.d.ts is byte-identical to before,
and the vue suite passes unchanged at 115 tests, including the
repeatable-group recursion that exercises this path. Full repo suite
green.
tsconfig.json claimed `target: ES2019`, but ng-packagr ignores it: the published fesm2022 bundle uses ES2022 class fields (`value;` declarations, define semantics). So the config described an emit that never shipped, and tsconfig.spec.json had to override target to ES2022 with a comment explaining it was matching reality -- an override that only existed to work around the wrong value here. Set the real target on the base config and let the spec config inherit it. Verified: the published fesm2022 bundle is byte-identical before and after, which is what confirms the old value was inert. Also drop `emitDecoratorMetadata`. Angular carries its own DI metadata on the generated `ɵfac`, and grepping the built bundle for `__metadata`, `Reflect.metadata` and `design:paramtypes` returns zero hits -- the flag produced nothing while implying a reflect-metadata dependency that does not exist.
…ounds
Angular's branch coverage sat at 75.8% against a 75 floor -- 0.8 points
of headroom, so the next uncovered `if` anyone added would have turned
CI red for whoever touched the file next rather than whoever wrote it.
The other three packages all had double-digit headroom, so this was
angular's gap, not a floor set too high.
The uncovered branches were real behaviour, not unreachable code:
- FieldInput.resolvedOptions (33% covered) decides between an explicit
`options` input, a static array on the field description, and a
dynamic options callback -- which it deliberately withholds, because
resolving it needs form data only MultiFieldInput has. Passing the raw
function down would make a renderer try to iterate a function. None of
those three paths was tested. Asserted through a renderer that prints
what it received, so the tests pin what reaches the renderer rather
than what the getter returns.
- MultiFieldInput's responsive layout (66%): mobile/desktop resolution,
the custom breakpoint (it queries `max-width: breakpoint - 1`, so a
viewport exactly at the breakpoint is desktop), the resize handler
only scheduling a re-render when the mobile state actually flips, and
the no-matchMedia fallback that keeps the component usable in jsdom.
Also grid column/gap defaults and the min/max item guards.
- DynamicFormDevTools.errorCount (50%): the `this.errors || {}` guard,
without which a null errors input takes the host app's render down.
Coverage: branches 75.8% -> 84.8% (54 -> 34 missed of 223), lines 91.4%
-> 95.4%, functions 92.7% -> 93.9%. Headroom over the floor goes from
+0.8 to +9.8, in line with core 85.9 / react 86.8 / vue 87.3. Suite goes
from 74 to 91 tests.
FieldInput and DynamicFormDevTools reach 100% branch coverage and
MultiFieldInput 92.9%, which is the evidence these tests hit the
branches they were written for rather than padding the number.
tsup emitted sourcemaps with sourcesContent, so each .map carried a full copy of the TypeScript source. That is what made them usable at all -- `files` publishes only `dist`, so a map pointing at ../src/*.ts would resolve to nothing -- but it also made them roughly half of every tarball, installed by every consumer. core 157.0 -> 83.0 KB unpacked (-47%), 33.3 -> 17.5 KB packed react 207.6 -> 90.1 KB unpacked (-57%), 46.8 -> 19.2 KB packed vue 250.4 -> 109.4 KB unpacked (-56%), 47.6 -> 20.5 KB packed Nothing that reaches an application bundle changes; sourcemaps never do. What changes is install size, against the ability to step into the library's TypeScript while debugging a consuming app. Calling it out plainly: this is a trade, not a free win. The maps were not broken or dead weight -- I checked, and sourcesContent made them fully self-contained. Each tsup.config.ts now carries that reasoning beside a `sourcemap: false` that is one word from restoring them. Verified no dangling //# sourceMappingURL comments survive in any dist, so nothing 404s looking for a map that is no longer shipped. angular is unaffected; ng-packagr's published output does not carry them.
Every run was annotating 10 warnings: actions/checkout@v4, setup-node@v4, upload-artifact@v4, download-artifact@v4 and codecov-action@v4 all declare `runs.using: node20`, which GitHub deprecated and now force-runs on Node 24 anyway. Bumped to the current majors, each verified against how this repo actually uses it rather than assumed: checkout v4 -> v7 setup-node v4 -> v7 upload-artifact v4 -> v7 download-artifact v4 -> v8 codecov-action v4 -> v7 upload-pages-artifact v3 -> v5 deploy-pages v4 -> v5 Two breaking changes in that range needed checking: - setup-node v6 limits *automatic* package-manager caching to npm. All seven call sites pass `cache: 'npm'` explicitly, so nothing relies on the detection that changed. - download-artifact v5 changed path behaviour for downloads **by ID**. Both call sites download by `name`, so the layout the verify and examples jobs depend on (`packages/<pkg>/dist`) is unaffected. Every input in use was confirmed to still exist in the new majors (`node-version-file`, `cache`, `name`, `path`, `retention-days`, `if-no-files-found`, `files`, `flags`, `fail_ci_if_error`), as was deploy-pages' `page_url` output that the Pages environment reads. upload-pages-artifact and deploy-pages were not in the warning list because the Pages workflow only runs on push to develop, not on a PR. Including them anyway: deploy-pages@v4 is node20, and upload-pages-artifact@v3 wraps upload-artifact@v4, so both would have warned on the next deploy. All four workflows re-parsed as valid YAML; the diff is version tags only.
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
1 participant
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.
Started as "check the current source", and each fix surfaced the next thing. Nothing here changes a public API; the two
perfcommits change what consumer bundlers can drop, and one commit changes what ships in the tarball.The two that were actually broken
npm audit --omit=devwas about to turn CI red on its own. A nanoid advisory (GHSA-2v37-7h3g-55p8) landed after the last green run ondevelop, reaching us throughvue → @vue/compiler-sfc → postcss. Root cause was classification: the root isprivateand ships nothing, but hadreact/react-dom/vueindependencies, so an--omit=devaudit walked vue's build-time tree. They move todevDependencies, and an override pins nanoid forward so the toolchain we run is actually patched rather than merely out of scope.scripts/check-cross-framework-imports.jshad never detected anything.isSourceFile()tested against/\.(ts|tsx|js|jsx)$/— in a regex literal\matches a backslash, not a dot — so the predicate returned false for every path, the walk callback returned early every time, and the gate printed "OK" unconditionally. It would have passed a package importing another framework wholesale. Verified by appending a cross-framework import topackages/vue/src/index.ts: before, exit 0 and "OK"; after, exit 1 naming the file and line. Running the fixed check against the repo finds no violations, so nothing had drifted while it was blind.Bundle size
Components were declared as bare top-level calls —
defineComponent({...}),React.memo(...)— which a bundler cannot prove side-effect free, so it evaluates them even when unused. Separately,MultiFieldInputreached itself for recursive group rendering through a module-scopemultiFieldInputSelfRef = MultiFieldInputassignment; a bare top-level assignment is a side effect nothing can drop, and it anchoredMultiFieldInput → FieldInput → DynamicInput → every default rendererinto any app that imported the package at all.Measured with esbuild, minified, framework external:
DynamicInputonlyDynamicInputonly"Everything" staying flat is the expected result — nothing is droppable when it is all reachable. Apps using the full surface pay 13–14 bytes for the wrapper function; the published
distgrows 0.03 KB (react) and 0.29 KB (vue) because the annotations are comments.dist/index.d.tsfor vue is byte-identical before and after, and the repeatable-group recursion tests pass unchanged.Install size
tsup emitted sourcemaps with
sourcesContent, so each.mapcarried a full copy of the TypeScript source. That is what made them work —filespublishes onlydist— but it also made them about half of every tarball: core 157.0 → 83.0 KB unpacked (−47%), react 207.6 → 90.1 KB (−57%), vue 250.4 → 109.4 KB (−56%).This is a deliberate trade, not a free win. The maps were fully functional. Nothing that reaches an application bundle changes; what changes is install size against the ability to step into library source while debugging. Each
tsup.config.tscarries the reasoning next to asourcemap: falsethat is one word from restoring them — worth a second opinion before merge.Angular coverage was 0.8 points from red
Branch coverage sat at 75.8% against a 75 floor. The other three packages all had double-digit headroom, so this was angular's gap, not a floor set too high. The uncovered branches were real behaviour:
FieldInput.resolvedOptions(33%) deciding between an explicit input, a static array, and a dynamic callback it deliberately withholds;MultiFieldInput's responsive layout (66%); andDynamicFormDevTools.errorCount's null guard.Branches 75.8% → 84.8%, lines 91.4% → 95.4%, suite 74 → 91 tests.
FieldInputandDynamicFormDevToolsreach 100% branch coverage, which is the evidence the tests hit the branches they were written for rather than padding the number.Smaller things
scripts/show-sizes.jsguessed CJS/ESM from file extensions, so it printed vue's ESM bundle under a "CJS" label, never printed vue's real CJS bundle, and omitted angular entirely. It now resolves entries from each package's own manifest.npm run lintonly coveredpackages/*/src, soscripts/was linted by the pre-commit hook but never in CI — the two gates disagreed. Extended, and the errors that surfaced are cleared.packages/vuepeer-depended onvuewithout dev-installing it, so it built and tested against whatever the root hoisted.verify-framework-deps.jsnow enforces that every peer is dev-declared, and reported exactly one violation — the one fixed here.packages/angular/tsconfig.jsonclaimedtarget: ES2019while ng-packagr emits ES2022 class fields, which is whytsconfig.spec.jsonhad to override it; andemitDecoratorMetadataproduced zero metadata. The published bundle is byte-identical after removing both, confirming they were inert.*.timestamp-*.mjswas ignored by neither.gitignorenor.prettierignore, so running the angular suite locally turned the nextformat-checkred..d.ts, but 25 public exports were undocumented. The substantive gap was thatvalidateField/validateFieldscannot await, so avalidatehook returning a Promise reads as valid — anduseDynamicForm/createDynamicFormStoreuse that sync path including on submit.New test infrastructure
scripts/had no tests. There is now a root vitest config scoped toscripts/**, atest:scriptsnpm script, and aTest build scriptsstep inquality-gates.ymlso it cannot rot. 19 tests across three files, built on throwaway workspaces on a real filesystem rather than mockingfs.Verification
Run at HEAD, all passing:
npm@10 ci,npm audit --omit=dev, lint (now includingscripts/), format-check, build, typecheck, core type tests,test:scripts, tests for all four packages, the published-package smoke suite, the three verify scripts, and builds of all three example apps.package-lock.jsonwas regenerated with npm@10 rather than the locally installed npm 11, which prunes other platforms' optional binaries: 1606 entries before and after, nothing added or removed, all 100 linux entries intact.Release note
Six changesets are pending, three of them new here. Worth knowing: the live-demo link fix landed on
developabout 25 hours after the last publish, so all four packages on npm still carry the oldgithub.com/vannt-dev/dynamic-field-kit-demolink in their README. That only reaches npm through a release.