fix(transforms): stop dropping import attributes on JSON modules (depends on #2999) - #3006
Conversation
80e8fe8 to
73333ce
Compare
20c3529 to
aacf9b5
Compare
kwakayama
left a comment
There was a problem hiding this comment.
Score: 88/100
The implementation is small and targeted, and the tests cover both preserving JSON import attributes through esbuild and removing the stale JSON attribute after a framework JSON dependency is rewritten to .mjs.
Not merge-ready yet:
- This is stacked on lower open PRs, including blocked/type-failing PRs.
- The PR status rollup only shows CLA, not full CI.
Watch item: stripJsonAttributesFromModuleImports is a regex over compiled output, so correctness is coupled to esbuild output shape. That is acceptable for this narrow case, but it should stay covered if more import attributes or output formats are introduced.
73333ce to
3ab5861
Compare
aacf9b5 to
85ab840
Compare
3ab5861 to
a3d1c39
Compare
85ab840 to
2487d05
Compare
2487d05 to
dcb6367
Compare
a3d1c39 to
a605762
Compare
dcb6367 to
aa8a5e7
Compare
a605762 to
b744d20
Compare
kwakayama
left a comment
There was a problem hiding this comment.
Follow-up approval after #2999 merged and the stack was rebased onto main.
Score: 92/100.
Rationale: import-attribute preservation is scoped and the rebased stack regression suite stayed green.
Next step: merge after lower stack PRs and refreshed checks are green.
1444152 to
eb196d5
Compare
1987423 to
783af4a
Compare
eb196d5 to
d9b21fb
Compare
783af4a to
34af0c0
Compare
d9b21fb to
4c2bb62
Compare
34af0c0 to
33e3873
Compare
4c2bb62 to
bbf0a11
Compare
56e5007 to
f85d6cf
Compare
bbf0a11 to
391d2ff
Compare
f85d6cf to
d6c07ca
Compare
391d2ff to
276b1fd
Compare
276b1fd to
03a1d2c
Compare
kwakayama
left a comment
There was a problem hiding this comment.
Follow-up approval after #3005 merged and #3006 was rebased onto main.
Score: 93/100.
Rationale: the transform preserves import attributes for JSON modules without widening unrelated import rewriting behavior, and focused regression coverage still passes after the rebase.
Verification:
- deno task audit
- combined targeted regression suite: 21 tests, 334 steps, 0 failed
Next step: wait for refreshed GitHub checks and required reviewer gate, then merge when green.
Constraint: JSON module imports require their `with { type: "json" }` attribute at runtime, even when source is lowered to older JavaScript targets.
Rejected: Raising the sanitizer opt-out baseline | tests can stop the shared esbuild service instead of disabling leak checks.
Confidence: high
Scope-risk: narrow
Directive: Keep import-attribute support centralized in shared esbuild transform options.
Tested: deno test --allow-all src/transforms/pipeline/stages/compile.test.ts
Tested: deno task lint:sanitizer-baseline
Tested: deno task lint
Tested: targeted regression suite: 18 tests, 303 steps, 0 failed
Tested: deno task audit
Not-tested: manual production deploy
03a1d2c to
69d29b9
Compare
kwakayama
left a comment
There was a problem hiding this comment.
Follow-up approval after fixing the sanitizer ratchet on the rebased #3006 head.
Score: 93/100.
Rationale: import attributes remain preserved, and the test now closes the esbuild service instead of adding sanitizer opt-outs.
Verification:
- deno test --allow-all src/transforms/pipeline/stages/compile.test.ts
- deno task lint:sanitizer-baseline
- deno task lint
- targeted regression suite: 18 tests, 303 steps, 0 failed
- deno task audit
Next step: wait for refreshed GitHub checks and required reviewer gate, then merge when green.
Summary
import { notFound } from "veryfront"— a documented public API — failed to load at all, 500-ing every page that used it:Two compounding causes:
1. esbuild was silently dropping the attribute. Every source transform lowers to
es2020/es2022, both of which pre-date import attributes, so esbuild droppedwith { type: "json" }rather than erroring. The framework barrel reaches#veryfront/server/dev-ui/manifest.json, whose attribute vanished. Verified directly:Fixed with an explicit
supported: { "import-attributes": true }, shared asESBUILD_SUPPORTED_FEATURESso every source transform agrees rather than eachtarget:site drifting.2. With the attribute preserved, the next failure was the opposite.
The framework resolver compiles a
.jsondependency into a cached.mjsthat default-exports the data, but left the attribute — which describes the original target — in place. Attributes on imports that now point at a.mjsare stripped.Reproduction
i-not-found.tsx,h-redirect.tsx,l-error-thrown.tsx— every page importing from"veryfront"src/transforms/pipeline/stages/compile.test.ts,src/transforms/pipeline/stages/ssr-vf-modules/transform.test.tsTest evidence
Covers attribute preservation on static and dynamic imports, and the strip pass (drops it for a
.mjstarget, keeps it for a genuine.jsontarget, single quotes, plain imports untouched).Wider suite:
deno task test:unit→ 2525 passed | 0 failed.SSR evidence
Emitted framework bundle, before → after:
Route status:
Note
h-redirect— the reproducer recordedredirect()as working at0.1.1094. It had since regressed to a 500 onmain; this restores it.Client evidence
Related
0.1.1094.Chain: this PR is part of a 13-PR chain fixing the bugs catalogued in
veryfront-router-testing.
Its base is the previous PR in the chain, so the diff shows only this fix.
The root of the chain is #2999 (
fix/ssr-lazy-import-graceful-degrade) — merge #2999 first, thenrebase the chain onto
main.Regression gate:
deno task test:unit→ 2525 passed | 0 failed;deno task lint,deno task fmt:checkanddeno task typecheckall clean. The reproducer's full 56-routematrix (
ROUTES.txt+sweep.sh) was re-run after every fix: 7 routes improved, 0 regressed.A 46-route Chromium hydration sweep (
client-sweep.mjs) backs the client-side claims.