fix(image): scan image imports via AST instead of regex - #1779
Conversation
The image-imports transform scanned raw source with a regex, which
matched `import X from '...img'` text anywhere it appeared, including
inside comments, strings, and template literals. When a commented-out
image import pointed at a file that exists on disk, the rewrite emitted
a `const X = { src: __vinext_img_url_X, ... }` referencing an undefined
variable, crashing SSR in dev and returning 500 in production.
Replace the regex scan with AST-based scanning via parseAst, rewriting
only real top-level ImportDeclaration nodes that use a single default
specifier and whose source ends in an image extension. The code regex
filter is kept purely as a cheap pre-check.
Add regression tests for commented-out imports (line and block),
image-import text inside string literals, and named/namespace imports.
|
/bigbonk review for issues |
commit: |
|
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
|
/bigbonk re-review for issues |
2 similar comments
|
/bigbonk re-review for issues |
|
/bigbonk re-review for issues |
|
/bigbonk review this pr for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(image): scan image imports via AST instead of regex
I verified the change against the actual parseAst in this repo (vite-plus / Rolldown+oxc). There is one critical regression that the new tests don't catch.
🔴 Critical: parseAst(code) throws on TSX/JSX/typed-TS, silently disabling image transforms
The vinext:image-imports plugin uses enforce: "pre" (packages/vinext/src/index.ts:3824), so its transform handler runs before the JSX/TS transform — i.e. on raw source containing JSX and TypeScript syntax.
But parseAst(code) (called with no options at index.ts:3888) only parses plain JavaScript in this environment. I confirmed this directly:
tsx => Parse failed: Unexpected JSX expression
ts => Parse failed: Missing initializer in const declaration (chokes on `const x: number = 5`)
js => PARSED OK
Because the handler catches the parse error and return null:
try {
ast = parseAst(code);
} catch {
return null; // <-- skips ALL image transforms for the file
}…any .tsx, .jsx, or .ts file with type annotations will skip image transformation entirely. The synthesized const X = { src, width, height } is never emitted, so import hero from './hero.png' resolves to a bare URL string and hero.src / hero.width / hero.height are undefined at runtime. This breaks the common case (local image imports in React components) to fix an edge case (commented-out imports).
Repro of the exact handler path on a realistic component:
filter matches (handler invoked): true
AST PARSE FAILED -> handler returns null, NO transform applied
Why CI is green: every test in tests/image-imports.test.ts passes only plain JS strings (no JSX, no : type annotations), even though fakeId is page.tsx. The 22 tests never exercise real TSX/TS syntax, so the regression is invisible to the suite.
This is notably the same hazard the neighboring vinext:use-cache plugin explicitly avoids — see the comment at index.ts:3967:
// Runs without enforce so it executes after JSX transform (parseAst needs plain JS).
image-imports cannot drop enforce: "pre" (it must rewrite the import before Vite resolves the asset), so it must instead tell the parser the input language.
Fix
Pass the TSX language to parseAst so it accepts JSX + TS + JS:
ast = parseAst(code, { lang: "tsx" });I verified { lang: "tsx" } parses all four input shapes (tsx/jsx/ts/js) with correct node start/end offsets, so s.overwrite(importNode.start, importNode.end, ...) stays accurate.
Required test additions
Add cases that use real TSX/TS syntax (the current tests would still pass even with the bug present):
- a
.tsxfile with JSX in the body + an image import → assertsexpectImageBinding(...)(currently fails) - a
.tsfile with a type annotation (e.g.const n: number = 1) + an image import → asserts the binding is rewritten
These two cases are what would have caught this before merge.
Minor / non-blocking
parseAstmay now run on every matching file in dev. Thecodefilter only narrows to files whose text contains an image-import-like pattern, but for those files you now always parse the full AST. That's the intended tradeoff (the PR body acknowledges "false positives there only cost a parse"), and it's fine — just confirming it's deliberate.- Behavior is otherwise correct. The AST-based gating (single
ImportDefaultSpecifier, string source, image extension) is the right shape, and ignoring named/namespace/side-effect imports matches the documented intent. Once thelangoption is added, the comment/string/template-literal regression cases are handled correctly.
Summary
The AST approach is the right direction and correctly fixes the commented-out-import crash, but as written it breaks image imports in all TSX/JSX/typed-TS files because parseAst is invoked without { lang: "tsx" } while the plugin runs enforce: "pre" on raw source. This needs the one-line lang fix plus TSX/TS regression tests before merge.
|
Review posted to PR #1779. Summary of findings: Critical issue found
The
Since the handler catches the parse error and Why CI stays green: all 22 tests in The fix is one line: The AST direction is correct and the commented-out-import fix is sound; it just needs the |
The image-imports plugin runs with enforce:"pre", so its transform sees
raw source before the JSX/TS transform. parseAst defaults to plain JS
and threw on JSX and TS type annotations; the handler swallowed the
error and returned null, silently skipping image transforms for every
.tsx/.jsx/typed-.ts file (hero.src/width/height became undefined).
Parse with { lang: "tsx" } so JSX + TS + JS all parse with correct node
offsets. Add regression tests using real TSX (JSX in body) and typed-TS
(type annotations) syntax that plain-JS parsing would reject.
|
Confirmed and fixed. The critical issue was real: the plugin runs with Changes in 6878e82:
I verified locally that both cases failed before the fix (handler returned null) and pass after. Full file is green (24 tests) and /bigbonk re-review for issues |
Always parsing as tsx broke plain .ts files: the tsx grammar reads `<T>` as the start of a JSX element, so TS-only syntax such as angle-bracket casts (`<Foo>bar`) and non-comma generic arrows (`<T>(x) => x`) threw during parse. The catch swallowed the error and returned null, silently skipping the image transform (hero.src/width/height become undefined). Select the language by extension: `.ts` parses as `ts`, everything else (.tsx/.jsx/.js/.mjs, which may contain JSX) parses as `tsx`. Add regression tests for the angle-bracket cast and generic-arrow cases.
|
Follow-up fix in 0f9c6fd: a self-review caught that always parsing as Now the parser language is selected by extension: /bigbonk re-review for issues |
There was a problem hiding this comment.
Re-review: fix(image): pick parser lang by extension (0f9c6fd)
I verified the follow-up fix against the actual parseAst (rolldown 1.0.0) in this repo. The fix is correct and the regression is real — approving.
Verified: the bug and the fix
parseAst's ParserOptions.lang accepts 'js' | 'jsx' | 'ts' | 'tsx' | 'dts' (rolldown binding type). I reproduced the exact tradeoff directly:
lang=ts cast (<{src}>v) => OK
lang=ts arrow (<T>(x)=>x) => OK
lang=ts jsx (<div/>) => THROW
lang=tsx cast => THROW ← the bug
lang=tsx arrow => THROW ← the bug
lang=tsx jsx => OK
So tsx and ts are genuinely mutually exclusive on <T>-ambiguous syntax, and selecting by extension (.ts → ts, everything else → tsx) is the right resolution. There is no single language that parses both JSX and angle-bracket casts, so extension dispatch is necessary, not just convenient.
Verified: the regression tests are genuine
I ran both new test inputs through the old (tsx-always) and new (ts) paths:
OLD(tsx) cast: THROW -> would skip transform (bug)
OLD(tsx) arrow: THROW -> would skip transform (bug)
NEW(ts) cast: OK
NEW(ts) arrow: OK
Both cases fail under the pre-fix behavior and pass after, so they actually guard the regression. Note the earlier .ts test (const count: number = 1) used only plain annotations, which tsx parses fine — that's why the <T> ambiguity slipped past the first round. The new cast/generic-arrow cases close that gap.
Verified: no query-string footgun in id.endsWith(".ts")
My one concern was that Vite often appends query suffixes (foo.ts?v=1), which would make id.endsWith(".ts") false and silently revert to tsx. This is not an issue here: both the transform filter.id.include (/\.(tsx?|jsx?|mjs)$/) and the handler's own defensive guard at index.ts:3875 are anchored with $, so any id that reaches the lang line at index.ts:3901 has already been confirmed to end in a bare extension with no query string. Safe as written.
Confirmed clean
vp test run tests/image-imports.test.ts→ 26 passedvp checkon both changed files → formatted, no lint/type errors
Minor / non-blocking (pre-existing, not introduced here)
.mts/.ctsare not handled. The include filter is/\.(tsx?|jsx?|mjs)$/— it matches.mjsbut not.mts/.cts, so a default image import in a.mtsfile would never be transformed. This is a pre-existing limitation of the filter (unchanged by this PR) and.mts/.ctsmodules rarely contain JSX or local image imports, so it's low priority. Worth a follow-up if you want full coverage: addm?[tj]sto both the include filter and the handler guard, and route.mts→ts.
Otherwise the change is tightly scoped, the comment accurately documents the <T> ambiguity, and the layering is fine for this small a transform. LGTM.
|
Re-review posted to PR #1779. SummaryThe follow-up fix in 0f9c6fd is correct — I approved it. Verified findings: The fix is sound. I confirmed against the actual rolldown The regression tests are genuine. Both the angle-bracket-cast and generic-arrow test inputs throw under the old No query-string footgun. My main concern was that Verified clean: 26 tests pass, One minor non-blocking note (pre-existing, not introduced here): the include filter |
|
/bigbonk re-review for issues |
3 similar comments
|
/bigbonk re-review for issues |
|
/bigbonk re-review for issues |
|
/bigbonk re-review for issues |
1 participant
Problem
The
vinext:image-importstransform scanned raw source text with a regex to find local image imports. A regex over raw text matchesimport X from '...img'anywhere the text appears — including inside comments, strings, and template literals.When a commented-out image import points at a file that exists on disk, the transform still matched it and rewrote the span via
MagicString.overwrite(). The first generated line stayed behind the//, but the synthesized meta-import andconstdeclaration escaped the comment, producing:__vinext_img_url_AbstractCatis never defined (its import is commented out), so SSR throwsReferenceError: __vinext_img_url_AbstractCat is not defined— page errors in dev and a 500 on every page in production.The bug only triggers when all of these hold:
import X from '...(image ext)'patternFix
Replace the regex scan with AST-based scanning using Vite's
parseAst. The handler now only rewrites real top-levelImportDeclarationnodes that:import X from '...')Comments, strings, template literals, named/namespace imports, and side-effect-only imports are all correctly ignored. The existing
coderegex filter is retained purely as a cheap pre-check for whether to invoke the handler (false positives there only cost a parse).Tests
Added regression cases to
tests/image-imports.test.ts:All 22 tests in the file pass;
vp checkis clean.