fix: skip MDX files during RSC scan-build to prevent parse errors#668
fix: skip MDX files during RSC scan-build to prevent parse errors#668Divkix wants to merge 3 commits intocloudflare:mainfrom
Conversation
commit: |
Divkix
force-pushed
the
fix/issue-659-mdx-scan-strip-parses-mdx
branch
from
43911ed to
cd696bb
Compare
There was a problem hiding this comment.
Pull request overview
Adds a protective plugin to avoid MDX parse failures during the App Router RSC scan/build flow, and introduces a regression test reproducing issue #659 (MDX loaded via import.meta.glob with YAML frontmatter + JSX).
Changes:
- Add
vinext:rsc-scan-mdx-guardto handle/skip.mdxduring RSC scan-build to preventrsc:scan-stripparse errors. - Add a regression test fixture that builds an App Router + Cloudflare environment using
import.meta.globover MDX with frontmatter/JSX.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
packages/vinext/src/index.ts |
Adds a pre-transform “guard” plugin intended to prevent rsc:scan-strip from parsing raw MDX during scan-build. |
tests/tsconfig-path-alias-build.test.ts |
Adds a build regression test covering MDX glob imports containing YAML frontmatter and JSX (issue #659). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/vinext/src/index.ts
Outdated
| name: "vinext:rsc-scan-mdx-guard", | ||
| enforce: "pre", | ||
| transform(code, id) { | ||
| if (!id.endsWith(".mdx")) return null; |
There was a problem hiding this comment.
if (!/\.mdx$/.test(id)) only matches ids that literally end with .mdx. In Vite builds ids commonly include queries (e.g. ?v=..., ?import) and RSC-related suffixes, so this guard may not run for many MDX requests. Consider normalizing the id (strip ?… / hash) before checking the extension, and include the .mdx + RSC-suffixed cases you expect to see in scan-build.
| if (!id.endsWith(".mdx")) return null; | |
| // Vite often appends queries (e.g. ?v=..., ?import, RSC flags) to | |
| // module ids. Normalize by stripping the query before checking the | |
| // extension so MDX files are correctly detected during scan-build. | |
| const queryIndex = id.indexOf("?"); | |
| const baseId = queryIndex === -1 ? id : id.slice(0, queryIndex); | |
| if (!baseId.endsWith(".mdx")) return null; |
…sue cloudflare#659) Adds a vinext:rsc-scan-mdx-guard plugin that runs with enforce:"pre" to ensure MDX files are skipped before rsc:scan-strip processes them. This prevents es-module-lexer parse errors when MDX files contain frontmatter (---) or JSX that isn't valid JavaScript syntax. Also adds a regression test that uses MDX files with YAML frontmatter loaded via import.meta.glob to verify the build succeeds.
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for the contribution and the test fixture — reproducing the issue is valuable. However, I believe the guard plugin as implemented is a no-op that doesn't actually fix the root cause, and the approach needs rethinking.
Core issue: the guard doesn't transform anything
The vinext:rsc-scan-mdx-guard plugin returns { code, map: null } — it passes through the same raw MDX source unchanged. rsc:scan-strip (which is enforce:"post") will still receive raw MDX and es-module-lexer.parse() will still fail on frontmatter --- and JSX.
Returning a non-null result from transform doesn't "claim" the file in a way that prevents later plugins from running. In Vite/Rolldown, transform hooks form a pipeline — every plugin's transform runs in order, each receiving the output of the previous one. So unless this guard replaces the MDX with valid JavaScript, rsc:scan-strip will still choke on it.
Why the test passes (likely)
The test includes app/mdx-probe/page.mdx — an MDX file inside app/. This triggers hasMdxFiles(), which causes vinext:config to set up mdxDelegate (auto-injecting @mdx-js/rollup). Once mdxDelegate is set, vinext:mdx transforms all .mdx files (including the ones under content/) into valid JS before rsc:scan-strip sees them. The guard plugin is irrelevant; vinext:mdx does the real work.
To prove this, try removing the app/mdx-probe/page.mdx file from the test fixture. If the guard were doing its job, the test should still pass. I suspect it will fail — because without the probe file, hasMdxFiles() returns false, mdxDelegate stays null, vinext:mdx is a no-op, the guard passes raw MDX through unchanged, and rsc:scan-strip crashes.
What the fix should actually do
The real fix needs to handle the case where MDX files enter the build graph (via import.meta.glob) but hasMdxFiles() didn't detect them (because they're outside app/ and pages/). Options:
-
Expand
hasMdxFiles()scanning — also scan directories that areimport.meta.globtargets, or just scan the entire project root for.mdxfiles (possibly expensive, but simpler). -
Make the guard actually transform MDX to valid JS — e.g., replace raw MDX with
export default {}or an empty module during scan-build (whenmanager.isScanBuildis true). This would require detecting scan-build context, which isn't straightforward from a non-RSC plugin. -
Ensure
vinext:mdxalways handles.mdxfiles — even withoutmdxDelegate, havevinext:mdxreturn a stub module for.mdxfiles during build, sorsc:scan-stripnever sees raw MDX.
Option 3 is probably the cleanest: if vinext:mdx sees an .mdx file and mdxDelegate is null, return export default function MDXContent() { return null; } instead of undefined. This makes the guard unnecessary.
packages/vinext/src/index.ts
Outdated
| enforce: "pre", | ||
| transform(code, id) { | ||
| if (!id.endsWith(".mdx")) return null; | ||
| return { code, map: null }; |
There was a problem hiding this comment.
This is a passthrough — it returns the same raw MDX code unchanged. Later plugins (including rsc:scan-strip at enforce:"post") still receive the raw MDX and es-module-lexer.parse() will still fail on frontmatter/JSX.
Vite's transform pipeline runs every plugin in order; returning a non-null result doesn't prevent downstream transforms from running. To actually protect against rsc:scan-strip, this would need to return valid JavaScript, e.g.:
| return { code, map: null }; | |
| if (!id.endsWith(".mdx")) return null; | |
| // Return a stub module so rsc:scan-strip's es-module-lexer.parse() | |
| // doesn't choke on raw MDX frontmatter/JSX syntax. | |
| return { code: "export default function MDXContent() { return null; }", map: null }; |
But even this approach has problems — it would clobber the real MDX content during non-scan builds too, since there's no isScanBuild check. I think the better fix is to handle this in vinext:mdx itself (see top-level review comment).
packages/vinext/src/index.ts
Outdated
| name: "vinext:rsc-scan-mdx-guard", | ||
| enforce: "pre", | ||
| transform(code, id) { | ||
| if (!id.endsWith(".mdx")) return null; |
There was a problem hiding this comment.
Copilot's suggestion to strip query strings is valid — Vite can append ?v=..., ?import, etc. to module IDs. id.endsWith(".mdx") won't match those. But this is secondary to the main issue that the passthrough doesn't actually fix anything.
| ); | ||
| writeFixtureFile( | ||
| root, | ||
| "app/mdx-probe/page.mdx", |
There was a problem hiding this comment.
This app/mdx-probe/page.mdx file is doing all the heavy lifting — it triggers hasMdxFiles() → sets up mdxDelegate → vinext:mdx compiles all .mdx files to JS. Without this file, the guard plugin alone would not prevent the crash.
To properly test the guard in isolation, try removing this probe file. If the test fails without it, that confirms the guard is a no-op and the fix needs a different approach.
Also, if the intent is that projects must always have an MDX file in app/ for MDX glob imports to work, that should be documented — but that would be a surprising requirement.
| const buildOutput = readTextFilesRecursive(path.join(root, "dist")); | ||
| expect(buildOutput).not.toContain('import.meta.glob("@/content/posts/**/*.mdx"'); | ||
| expect(buildOutput).not.toContain("@/content/posts/"); | ||
| expect(buildOutput).not.toContain("---"); |
There was a problem hiding this comment.
This assertion is fragile:
expect(buildOutput).not.toContain("---");--- is extremely common in generated JavaScript (think of comment separators, string literals, minified code). This could produce false negatives. Consider asserting on the full frontmatter block or on a more specific marker like title: "Second Post" instead.
|
Review submitted. Here's a summary of the feedback: Requested changes on PR #668. The main finding is that the
|
Divkix
force-pushed
the
fix/issue-659-mdx-scan-strip-parses-mdx
branch
from
cd696bb to
e38d65b
Compare
The guard plugin was a no-op that passed raw MDX through unchanged. When rsc:scan-strip received this raw MDX, es-module-lexer.parse() failed on frontmatter (---) and JSX syntax. The real fix is in vinext:mdx: when mdxDelegate is null (no MDX files in app/ or pages/), return a valid JS stub module instead of undefined. This handles MDX files that enter the build graph via import.meta.glob outside app/pages. - Remove vinext:rsc-scan-mdx-guard plugin (was a no-op) - Modify vinext:mdx to return stub module when mdxDelegate is null - Remove app/mdx-probe/page.mdx from test (was hiding the bug) - Replace fragile '---' assertion with specific frontmatter checks Fixes issue cloudflare#659
|
/bigbonk review |
There was a problem hiding this comment.
Thanks for iterating on this — the revised approach of returning a stub module from vinext:mdx when mdxDelegate is null is the right direction (and addresses the bonk feedback about the passthrough being a no-op). However, there are a few issues that need fixing before this can merge.
Summary of issues
- Dead code from query-stripping logic — lines 2599-2600 are unreachable because the early return at line 2597 already handles all IDs with query strings.
- Removing
mdx-probefrom test 1 silently changes what it tests — this was the existing test's mechanism for triggering@mdx-js/rollup. Without it, test 1 now exercises the stub fallback instead of the real MDX delegate, which is a behavioral change to an existing test. - Missing positive assertion that the stub was used — the new test verifies raw MDX is absent from output, but doesn't confirm the stub path was actually taken. A false pass is possible if some other mechanism handles the MDX.
- Stub silently discards MDX content at runtime — when
mdxDelegateis null, users get an empty component with no warning. This is fine for scan-build, but problematic for real builds where users expect their MDX content to render.
packages/vinext/src/index.ts
Outdated
| const queryIndex = id.indexOf("?"); | ||
| const baseId = queryIndex === -1 ? id : id.slice(0, queryIndex); | ||
| if (!baseId.endsWith(".mdx")) return; |
There was a problem hiding this comment.
These three lines are dead code. The early return at line 2597 (if (id.includes("?")) return;) already bails out for any ID containing a query string. If execution reaches line 2599, id.indexOf("?") will always be -1 and baseId will always equal id.
Either:
- Remove the dead query-stripping code and just use
if (!id.endsWith(".mdx")) return;, or - Remove the early return at line 2597 and rely solely on the query-stripping logic (but then you'd need to also skip query-bearing IDs for the delegate path, which
@mdx-js/rollupmay not handle correctly)
The simplest fix:
| const queryIndex = id.indexOf("?"); | |
| const baseId = queryIndex === -1 ? id : id.slice(0, queryIndex); | |
| if (!baseId.endsWith(".mdx")) return; | |
| if (!id.endsWith(".mdx")) return; |
| export default function HomePage() { | ||
| return <main>home {getGlobPostCount()}</main>; | ||
| } | ||
| `, | ||
| ); | ||
| writeFixtureFile( | ||
| root, | ||
| "app/mdx-probe/page.mdx", | ||
| `# Probe | ||
|
|
||
| This file exists only to trigger vinext's MDX auto-detection in the fixture. | ||
| `, |
There was a problem hiding this comment.
This removal changes the semantics of the existing first test. Previously, app/mdx-probe/page.mdx triggered hasMdxFiles() → mdxDelegate was set → @mdx-js/rollup compiled the MDX files. Without it, the first test now silently exercises the new stub fallback instead of the real MDX compilation pipeline.
This is unintentional — the first test should continue to test the happy path where @mdx-js/rollup is properly auto-detected. Please restore the mdx-probe file in the first test. The second test (issue #659) is the one that should exercise the no-delegate stub path.
packages/vinext/src/index.ts
Outdated
| // No MDX plugin registered — return a stub module so rsc:scan-strip | ||
| // doesn't choke on raw MDX frontmatter/JSX. This handles the case where | ||
| // MDX files enter the build graph via import.meta.glob but aren't inside | ||
| // app/ or pages/ (so hasMdxFiles() didn't detect them). | ||
| return { | ||
| code: `export default function MDXContent() { return null; }`, | ||
| map: null, |
There was a problem hiding this comment.
Returning a silent stub is reasonable for preventing parse errors, but it means users whose MDX files are only outside app//pages/ will get empty components with no indication why their content isn't rendering.
Consider adding a console.warn so users know MDX compilation was skipped:
| // No MDX plugin registered — return a stub module so rsc:scan-strip | |
| // doesn't choke on raw MDX frontmatter/JSX. This handles the case where | |
| // MDX files enter the build graph via import.meta.glob but aren't inside | |
| // app/ or pages/ (so hasMdxFiles() didn't detect them). | |
| return { | |
| code: `export default function MDXContent() { return null; }`, | |
| map: null, | |
| // No MDX plugin registered — return a stub module so rsc:scan-strip | |
| // doesn't choke on raw MDX frontmatter/JSX. This handles the case where | |
| // MDX files enter the build graph via import.meta.glob but aren't inside | |
| // app/ or pages/ (so hasMdxFiles() didn't detect them). | |
| console.warn( | |
| `[vinext] MDX file ${id} was stubbed because @mdx-js/rollup was not auto-detected. ` + | |
| `Add an .mdx file to your app/ or pages/ directory, or register @mdx-js/rollup manually.`, | |
| ); | |
| return { | |
| code: `export default function MDXContent() { return null; }`, | |
| map: null, | |
| }; |
Alternatively (and perhaps better long-term), expand hasMdxFiles() to also scan the full project root for .mdx files (or at least the glob targets), so that mdxDelegate gets set and MDX is properly compiled instead of stubbed.
| // These should NOT appear as raw strings in the build output | ||
| expect(buildOutput).not.toContain('title: "Second Post"'); | ||
| expect(buildOutput).not.toContain('date: "2025-08-20"'); | ||
| // JSX from the MDX file (text-red-500 className) should be compiled away | ||
| // Note: className= appears in React runtime itself, so we check the specific string | ||
| expect(buildOutput).not.toContain("text-red-500"); |
There was a problem hiding this comment.
These assertions verify that raw MDX content is absent from the build output, which is good. But they don't positively confirm how the MDX was handled. The test would also pass if the MDX files were simply excluded from the build graph entirely (e.g., by a glob resolution bug).
Consider adding a positive assertion that the stub function appears in the output, e.g.:
// Verify the stub module was used (since no mdx-probe triggers hasMdxFiles)
expect(buildOutput).toContain("MDXContent");This confirms the stub path was actually taken rather than the files being silently dropped.
|
Review submitted. Here's a summary of the feedback I posted: Requested changes on PR #668 with 4 inline comments:
|
Compile plain .mdx files on demand when they enter the graph via import.meta.glob outside app/pages so scan-build stops crashing without discarding runtime content. Restore the existing happy-path MDX fixture, add focused lazy-compilation coverage, and extract shared Cloudflare fixture setup to keep the regression tests honest.
|
/bigbonk review |
There was a problem hiding this comment.
Review
The approach has improved significantly across the 3 iterations. The current version — lazily initializing @mdx-js/rollup on-demand when an .mdx file hits the transform hook without being pre-detected by hasMdxFiles() — is the right fix. It addresses both prior bonk reviews: the guard is no longer a passthrough, and it properly compiles MDX rather than returning a stub.
The refactoring of the build test into shared helpers (writeCloudflareAppFixture, buildCloudflareAppFixture) is clean and makes it easy to add the regression test.
A few things worth addressing before merge:
| } | ||
| const delegate = mdxFactory(mdxOpts); | ||
| mdxDelegate = delegate; | ||
| if (reason === "detected") { |
There was a problem hiding this comment.
The reason parameter captures what triggered delegate creation, but it's only used for the log message. If two concurrent .mdx transforms race into ensureMdxDelegate, the first one creates the promise and the second reuses it via mdxDelegatePromise. That's correct for dedup.
However, the reason is captured inside the closure at creation time, so if the first call is "detected" and a concurrent "on-demand" call arrives, only "detected" is logged. This is fine in practice (the config hook runs before transforms), but worth noting.
| let warnedMissingMdxPlugin = false; | ||
|
|
||
| async function ensureMdxDelegate(reason: "detected" | "on-demand"): Promise<Plugin | null> { | ||
| if (mdxDelegate || hasUserMdxPlugin) return mdxDelegate; |
There was a problem hiding this comment.
When hasUserMdxPlugin is true, this returns mdxDelegate (which is null). The caller in transform then falls through to the if (!hasUserMdxPlugin) check and silently returns undefined, correctly deferring to the user's plugin. The logic is correct but a bit subtle — a brief comment explaining this path would help readability:
| if (mdxDelegate || hasUserMdxPlugin) return mdxDelegate; | |
| // If user registered their own MDX plugin, don't interfere — their plugin | |
| // will handle .mdx transforms later in the pipeline. | |
| if (mdxDelegate || hasUserMdxPlugin) return mdxDelegate; |
| console.log("[vinext] Auto-injected @mdx-js/rollup for on-demand MDX support"); | ||
| } | ||
| return delegate; | ||
| } catch { |
There was a problem hiding this comment.
When ensureMdxDelegate("on-demand") is called and @mdx-js/rollup is not installed, this returns null. Back in the transform hook (line 2623), delegate is null, so it skips to line 2630 and throws an error. That error message says "no MDX plugin is configured", which is accurate.
But there's a subtle issue: the console.warn at line 1350 fires first (from the catch block), then the transform throws. The user sees both a warning and an error for the same root cause. Consider either:
- Suppressing the warn when the throw will follow (since the error is more actionable), or
- Not throwing in the transform and just returning the warn (though that would leave raw MDX in the pipeline, which is worse)
The current behavior (warn + throw) is fine — the throw is what actually prevents the broken build — but the double messaging is slightly noisy.
| `, | ||
| ); | ||
|
|
||
| await buildCloudflareAppFixture(root); |
There was a problem hiding this comment.
This test correctly exercises the issue #659 scenario: MDX with YAML frontmatter and JSX enters the build graph via import.meta.glob without any .mdx file in app/ or pages/ (so hasMdxFiles() returns false). The on-demand lazy init path is the only thing that can save the build here.
However, the assertions at lines 310-311 only verify that alias paths were resolved. They don't positively confirm that the MDX was actually compiled (vs. silently dropped). Consider adding:
// Verify MDX was compiled — the frontmatter content should not appear as raw YAML
expect(buildOutput).not.toContain('title: "Second Post"');
// Verify the compiled MDX produced output (not silently dropped)
expect(buildOutput).toContain('text-red-500');The text-red-500 string from the JSX <span className="text-red-500"> should survive MDX compilation into the build output, confirming the content was actually processed rather than silently excluded.
|
|
||
| expect(result).toBeDefined(); | ||
| expect(result.code).toContain("mdx-evaluated"); | ||
| expect(result.code).not.toContain("---"); |
There was a problem hiding this comment.
expect(result.code).not.toContain("---") is fragile — --- can appear in compiled JS output (comment separators, minified expressions, template literals). In this unit test it's likely fine because the output is unminified MDX compilation output, but consider matching the full frontmatter delimiter or the actual YAML content instead:
| expect(result.code).not.toContain("---"); | |
| expect(result.code).not.toContain('title: "Second Post"'); |
This directly asserts the frontmatter was stripped rather than relying on --- not appearing anywhere in the output.
| `, | ||
| ); | ||
| ); | ||
| writeFixtureFile( |
There was a problem hiding this comment.
Good addition — mdx-components.tsx is needed for @mdx-js/rollup to work properly with the MDX component resolution. Having it in the shared fixture setup ensures both tests get it.
|
Review posted successfully. Here's a summary of the feedback: COMMENT review on PR #668 with 6 inline comments:
Overall the approach is solid — lazy on-demand initialization of |
Summary
vinext:rsc-scan-mdx-guardplugin that skips.mdxfiles during RSC scan-buildrsc:scan-stripfrom trying to parse MDX frontmatter/JSX as JavaScriptrsc:scan-stripparses MDX files as JS before@mdx-js/rolluptransforms them, causing parse errors #659 with MDX files containing YAML frontmatterRoot cause
When
import.meta.globloads MDX files in an RSC build,rsc:scan-strip(from@vitejs/plugin-rsc) runs on all files including MDX. It useses-module-lexer.parse()which fails on MDX frontmatter (---) and JSX syntax.Testing
pnpm test tests/tsconfig-path-alias-build.test.ts- 2 tests passpnpm test tests/app-router.test.ts- 271 tests passpnpm test tests/routing.test.ts- 91 tests passCloses #659