Skip to content

fix: skip MDX files during RSC scan-build to prevent parse errors #668

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Divkix wants to merge 7 commits into
base: main
Choose a base branch
from
Open

fix: skip MDX files during RSC scan-build to prevent parse errors #668

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e38d65b
fix: skip MDX files during RSC scan-build to prevent parse errors (is…
Divkix Mar 23, 2026
af6f644
fix(mdx): return stub module for undetected MDX files during scan-build
Divkix Mar 24, 2026
f873091
fix(mdx): lazily compile globbed mdx imports
Divkix Mar 25, 2026
a0a2b05
fix review issues
Divkix Mar 25, 2026
722a144
fix: use absolute path for root tsconfig alias to fix import.meta.glo…
Divkix Mar 25, 2026
f5f2bdb
refactor: address review feedback on MDX lazy initialization
Divkix Mar 28, 2026
3ed5fe8
refactor: address review feedback on MDX handling
Divkix Mar 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 87 additions & 39 deletions packages/vinext/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,9 @@ function toViteAliasReplacement(absolutePath: string, projectRoot: string): stri

for (const rootCandidate of rootCandidates) {
for (const pathCandidate of pathCandidates) {
if (pathCandidate === rootCandidate) return "/";
if (pathCandidate === rootCandidate) {
return normalizedPath;
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a meaningful behavioral change that's easy to miss in the diff. Previously, when the alias target resolved to the project root (e.g., @/* -> ./* with baseUrl: "."), this returned "/". Now it returns the full normalized absolute path.

The old "/" was problematic because Vite's resolve.alias can interpret "/" as the filesystem root, not the project root. Returning the actual absolute path is correct.

However, this change affects all tsconfig alias resolution, not just the MDX/test scenario. It should be called out in the PR description since it's a separate fix from the MDX issue.

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a separate behavioral change from the MDX fix and should be called out in the PR description. Previously, when the alias target resolved to the project root (e.g., @/* -> ./* with baseUrl: "."), this returned "/". Now it returns the full normalized absolute path.

The new behavior is correct — Vite's resolve.alias can interpret "/" as the filesystem root on Linux, not the project root. But this affects all tsconfig alias resolution, not just the MDX/test scenario. Users relying on the old "/" behavior (unlikely, but possible) could see different resolution results.

Please add a line to the PR description noting this as a companion fix.

}
const relativeId = relativeWithinRoot(rootCandidate, pathCandidate);
if (relativeId) return "/" + relativeId;
}
Expand Down Expand Up @@ -1307,9 +1309,69 @@ export default function vinext(options: VinextOptions = {}): PluginOption[] {

const imageImportDimCache = new Map<string, { width: number; height: number }>();

// Shared state for the MDX proxy plugin. Populated during config() if MDX
// files are detected and @mdx-js/rollup is installed.
// Shared state for the MDX proxy plugin. We auto-inject @mdx-js/rollup when
// MDX is detected in app/pages during config(), and lazily on first plain
// .mdx transform for MDX that only enters the graph via import.meta.glob.
let mdxDelegate: Plugin | null = null;
// Cached across calls — only the first invocation's `reason` affects logging.
// This is correct because config() always runs before transform() in the same build.
let mdxDelegatePromise: Promise<Plugin | null> | null = null;
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note on the caching semantics: mdxDelegatePromise is set on the first call to ensureMdxDelegate and reused by all subsequent calls. This means the reason parameter only affects the first invocation's log message. If ensureMdxDelegate("detected") runs first (from config), fails, and caches a resolved null, then a later ensureMdxDelegate("on-demand") call from the transform hook will return that cached null without logging. The transform then throws with a clear error message, so the behavior is correct — but the reason parameter creates a slight impression that retries might differ.

Consider adding a comment on this line:

Suggested change
let mdxDelegatePromise: Promise<Plugin | null> | null = null;
// Cached across calls — only the first invocation's `reason` affects logging.
let mdxDelegatePromise: Promise<Plugin | null> | null = null;

let hasUserMdxPlugin = false;
let warnedMissingMdxPlugin = false;

async function ensureMdxDelegate(reason: "detected" | "on-demand"): Promise<Plugin | null> {
// If user registered their own MDX plugin, don't interfere — their plugin
// will handle .mdx transforms later in the pipeline.
// Note: hasUserMdxPlugin is set during config() which always runs before transform().
// When true and mdxDelegate is still null (no auto-injected delegate), the
// caller in transform() silently returns undefined to let the user's plugin handle the file.
if (mdxDelegate || hasUserMdxPlugin) return mdxDelegate;
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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;

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The control flow here is correct but subtle. When hasUserMdxPlugin is true, this returns mdxDelegate (which is null). The caller in transform then falls through to the if (!hasUserMdxPlugin) check at line 2634, which is false, so it silently returns undefined — correctly deferring to the user's plugin.

A brief comment would help readability:

Suggested change
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.
// Note: hasUserMdxPlugin is set during config() which always runs before transform().
if (mdxDelegate || hasUserMdxPlugin) return mdxDelegate;

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: the comment block above (lines 1323-1327) is good and addresses earlier review feedback about this subtle control flow. One nit — the comment says "When true, we return null (mdxDelegate is null)" but technically if mdxDelegate is non-null and hasUserMdxPlugin is also true (which shouldn't happen in practice), it would return the delegate. The intended path is the hasUserMdxPlugin branch, so the comment is fine for practical purposes, just slightly imprecise.

if (!mdxDelegatePromise) {
mdxDelegatePromise = (async () => {
try {
const mdxRollup = await import("@mdx-js/rollup");
const mdxFactory = (mdxRollup.default ?? mdxRollup) as (
options: Record<string, unknown>,
) => Plugin;
const mdxOpts: Record<string, unknown> = {};
if (nextConfig.mdx) {
if (nextConfig.mdx.remarkPlugins) mdxOpts.remarkPlugins = nextConfig.mdx.remarkPlugins;
if (nextConfig.mdx.rehypePlugins) mdxOpts.rehypePlugins = nextConfig.mdx.rehypePlugins;
if (nextConfig.mdx.recmaPlugins) mdxOpts.recmaPlugins = nextConfig.mdx.recmaPlugins;
}
const delegate = mdxFactory(mdxOpts);
mdxDelegate = delegate;
if (reason === "detected") {
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (nextConfig.mdx) {
console.log(
"[vinext] Auto-injected @mdx-js/rollup with remark/rehype plugins from next.config",
);
} else {
console.log("[vinext] Auto-injected @mdx-js/rollup for MDX support");
}
} else {
console.log("[vinext] Auto-injected @mdx-js/rollup for on-demand MDX support");
}
return delegate;
} catch {
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Only warn during "detected" path (MDX files in app/pages at config time).
// For "on-demand" (MDX encountered during transform), the error thrown
// in transform() is more actionable and immediate. Avoid double messaging.
if (reason === "detected" && !warnedMissingMdxPlugin) {
warnedMissingMdxPlugin = true;
console.warn(
"[vinext] MDX files detected but @mdx-js/rollup is not installed. " +
"Install it with: " +
detectPackageManager(process.cwd()) +
" @mdx-js/rollup",
);
}
return null;
}
})();
}
return mdxDelegatePromise;
}

const plugins: PluginOption[] = [
// Resolve tsconfig paths/baseUrl aliases so real-world Next.js repos
Expand Down Expand Up @@ -2035,45 +2097,18 @@ export default function vinext(options: VinextOptions = {}): PluginOption[] {

// Auto-inject @mdx-js/rollup when MDX files exist and no MDX plugin is
// already configured. Applies remark/rehype plugins from next.config.
const hasMdxPlugin = pluginsFlat.some(
hasUserMdxPlugin = pluginsFlat.some(
(p: any) =>
p &&
typeof p === "object" &&
typeof p.name === "string" &&
(p.name === "@mdx-js/rollup" || p.name === "mdx"),
);
if (
!hasMdxPlugin &&
!hasUserMdxPlugin &&
hasMdxFiles(root, hasAppDir ? appDir : null, hasPagesDir ? pagesDir : null)
) {
try {
const mdxRollup = await import("@mdx-js/rollup");
const mdxFactory = mdxRollup.default ?? mdxRollup;
const mdxOpts: Record<string, unknown> = {};
if (nextConfig.mdx) {
if (nextConfig.mdx.remarkPlugins)
mdxOpts.remarkPlugins = nextConfig.mdx.remarkPlugins;
if (nextConfig.mdx.rehypePlugins)
mdxOpts.rehypePlugins = nextConfig.mdx.rehypePlugins;
if (nextConfig.mdx.recmaPlugins) mdxOpts.recmaPlugins = nextConfig.mdx.recmaPlugins;
}
mdxDelegate = mdxFactory(mdxOpts);
if (nextConfig.mdx) {
console.log(
"[vinext] Auto-injected @mdx-js/rollup with remark/rehype plugins from next.config",
);
} else {
console.log("[vinext] Auto-injected @mdx-js/rollup for MDX support");
}
} catch {
// @mdx-js/rollup not installed — warn but don't fail
console.warn(
"[vinext] MDX files detected but @mdx-js/rollup is not installed. " +
"Install it with: " +
detectPackageManager(process.cwd()) +
" @mdx-js/rollup",
);
}
await ensureMdxDelegate("detected");
}

// Detect if this is a standalone SSR build (set by `vite build --ssr`
Expand Down Expand Up @@ -2591,14 +2626,27 @@ export default function vinext(options: VinextOptions = {}): PluginOption[] {
const fn = typeof hook === "function" ? hook : hook.handler;
return fn.call(this, config, env);
},
transform(code, id, options) {
async transform(code, id, options) {
// Skip ?raw and other query imports — @mdx-js/rollup ignores the query
// and would compile the file as MDX instead of returning raw text.
if (id.includes("?")) return;
if (!mdxDelegate?.transform) return;
const hook = mdxDelegate.transform;
const fn = typeof hook === "function" ? hook : hook.handler;
return fn.call(this, code, id, options);
// Case-insensitive extension check for cross-platform compatibility
// (Windows/macOS case-insensitive, Linux case-sensitive)
if (!id.toLowerCase().endsWith(".mdx")) return;
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The case-insensitive check (id.toLowerCase().endsWith(".mdx")) is a reasonable defensive measure for cross-platform compatibility. However, note that this is the only place in the plugin that does case-insensitive extension matching — the hasMdxFiles() function and the config() hook's MDX detection both use case-sensitive checks. If you're going to be defensive here, it might be worth being consistent elsewhere too. Not a blocker, just something to be aware of.


const delegate = mdxDelegate ?? (await ensureMdxDelegate("on-demand"));
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mdxDelegate ?? short-circuit is a nice fast-path that avoids the async ensureMdxDelegate call on subsequent .mdx files after the delegate is already initialized. ensureMdxDelegate also checks mdxDelegate at line 1323 and returns it synchronously, but you'd still pay the cost of entering the async function. Good optimization.

if (delegate?.transform) {
const hook = delegate.transform;
const transform = typeof hook === "function" ? hook : hook.handler;
return transform.call(this, code, id, options);
}

if (!hasUserMdxPlugin) {
throw new Error(
`[vinext] Encountered MDX module ${id} but no MDX plugin is configured. ` +
`Install @mdx-js/rollup or register an MDX plugin manually.`,
);
}
},
},
// Shim React canary/experimental APIs (ViewTransition, addTransitionType)
Expand Down Expand Up @@ -4944,7 +4992,7 @@ function scanDirForMdx(dir: string): boolean {
const full = path.join(dir, entry.name);
if (entry.isDirectory()) {
if (scanDirForMdx(full)) return true;
} else if (entry.isFile() && entry.name.endsWith(".mdx")) {
} else if (entry.isFile() && entry.name.toLowerCase().endsWith(".mdx")) {
return true;
}
}
Expand Down
62 changes: 56 additions & 6 deletions tests/pages-router.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1331,7 +1331,7 @@ describe("Plugin config", () => {
expect(defaultHandler).not.toHaveBeenCalled();
});

it("registers vinext:mdx proxy plugin with enforce pre for correct ordering", () => {
it("registers vinext:mdx proxy plugin with enforce pre for correct ordering", async () => {
const plugins = vinext() as any[];
const mdxProxy = plugins.find((p) => p.name === "vinext:mdx");
expect(mdxProxy).toBeDefined();
Expand All @@ -1341,20 +1341,70 @@ describe("Plugin config", () => {
expect(typeof mdxProxy.transform).toBe("function");
// Proxy should be inert when no MDX files are detected (mdxDelegate is null)
expect(mdxProxy.config({}, { command: "build", mode: "production" })).toBeUndefined();
expect(mdxProxy.transform("code", "./foo.ts", {})).toBeUndefined();
await expect(mdxProxy.transform("code", "./foo.ts", {})).resolves.toBeUndefined();
});

it("vinext:mdx transform skips ids that contain a query string (regression: ?raw)", () => {
it("vinext:mdx transform skips ids that contain a query string (regression: ?raw)", async () => {
// @mdx-js/rollup strips the query before matching the file extension, so
// it would compile "foo.mdx?raw" as MDX and return compiled JSX instead of
// raw text. The proxy must short-circuit on any id that contains "?".
const plugins = vinext() as any[];
const mdxProxy = plugins.find((p: any) => p.name === "vinext:mdx");

// Common query-param import patterns that must be skipped
expect(mdxProxy.transform("# hello", "/app/content.mdx?raw", {})).toBeUndefined();
expect(mdxProxy.transform("# hello", "/app/page.mdx?url", {})).toBeUndefined();
expect(mdxProxy.transform("# hello", "/app/page.mdx?inline", {})).toBeUndefined();
await expect(
mdxProxy.transform("# hello", "/app/content.mdx?raw", {}),
).resolves.toBeUndefined();
await expect(mdxProxy.transform("# hello", "/app/page.mdx?url", {})).resolves.toBeUndefined();
await expect(
mdxProxy.transform("# hello", "/app/page.mdx?inline", {}),
).resolves.toBeUndefined();
// Additional query variations
await expect(mdxProxy.transform("# hello", "/app/page.mdx?v=123", {})).resolves.toBeUndefined();
await expect(mdxProxy.transform("# hello", "/app/page.mdx?mdx", {})).resolves.toBeUndefined();
// Edge case: query value contains .mdx but isn't the extension
await expect(
mdxProxy.transform("# hello", "/app/page.mdx?something.mdx", {}),
).resolves.toBeUndefined();
});

it("vinext:mdx lazily compiles plain .mdx imports that were not pre-detected", async () => {
const tmpDir = await fsp.mkdtemp(path.join(os.tmpdir(), "vinext-mdx-lazy-"));

try {
await fsp.writeFile(
path.join(tmpDir, "package.json"),
JSON.stringify({ name: "vinext-mdx-lazy", private: true, type: "module" }),
);

const plugins = vinext({ appDir: tmpDir }) as any[];
const configPlugin = plugins.find((p) => p.name === "vinext:config");
const mdxProxy = plugins.find((p) => p.name === "vinext:mdx");

await configPlugin.config(
{ root: tmpDir, plugins: [] },
{ command: "build", mode: "production" },
);

const result = await mdxProxy.transform(
`---
title: "Second Post"
---

export const marker = "mdx-evaluated";

# Hello <span>world</span>
`,
path.join(tmpDir, "content", "post.mdx"),
{},
);

expect(result).toBeDefined();
expect(result.code).toContain("mdx-evaluated");
expect(result.code).not.toContain('title: "Second Post"');
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good improvement over the earlier not.toContain("---") approach. Asserting on the actual YAML content 'title: "Second Post"' is specific and won't false-positive from JS output containing dashes.

} finally {
await fsp.rm(tmpDir, { recursive: true, force: true });
}
});

it("vinext:mdx proxy logic — ?raw guard prevents delegate from compiling query imports", () => {
Expand Down
Loading
Loading