fix: strip server-only data-fetching exports from client bundles#282
fix: strip server-only data-fetching exports from client bundles#282southpolesteve merged 2 commits intomainfrom
Conversation
|
/bigbonk review this |
commit: |
|
|
@southpolesteve Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review this |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good idea — this is a necessary feature for Pages Router client bundles. Next.js's SWC transform (next-ssg-transform) does this, and vinext needs an equivalent. The test coverage is solid for the happy paths.
However, the regex-based approach has several correctness bugs that will silently produce broken client bundles for common real-world patterns. I verified these by running _stripServerExports against each case.
Bugs found
1. Function expressions (= function() {}) produce broken output
export const getStaticProps = function() {
const data = fetchData();
return { props: { data } };
};The arrow regex doesn't match function() {. It falls through to the "simple assignment" branch which uses indexOf(";") — this finds the first semicolon inside the function body, producing:
export const getStaticProps = undefined;
return { props: { data } };
};Same bug affects export const getStaticProps = async function name() { ... }.
Next.js's transform handles both patterns (see test/unit/babel-plugin-next-ssg-transform.test.ts lines ~130-170).
2. Arrow functions with TS return type annotations break the regex
export const getServerSideProps = async (ctx: Ctx): Promise<Result> => {
// ...
};The arrow regex uses \([^)]*\) which matches the param list, but then expects \s*=>\s*\{ immediately after ). The return type annotation (: Promise<Result>) sits between ) and =>, so the regex fails. Falls through to indexOf(";") with the same truncation bug.
3. export { getServerSideProps } re-export syntax is unhandled
const getServerSideProps = fetchData;
export { getServerSideProps };Only export function and export const/let/var are matched. Re-export syntax is missed entirely. The function returns null (no transform), so the server-only import survives in the client bundle.
Next.js's transform explicitly handles this (see test/unit/babel-plugin-next-ssg-transform.test.ts lines ~30-50 and ~180-260).
4. Regex literals with braces confuse skipBalanced
export function getServerSideProps() {
const pattern = /\{[^}]+\}/;
return { props: {} };
}skipBalanced handles strings and comments but not regex literals. The } inside /[^}]+/ decrements the brace depth, causing premature "close" of the function body.
5. Expression-body arrows are fragile
export const getStaticPaths = () => [
{ params: { id: 'a;b' } },
];This happens to work for simple cases because indexOf(";") finds the ; after ]. But if any element contains a semicolon in a string, it breaks.
Recommendation
Consider using Vite's parseAst (already imported at line 2) to do an AST-based transform instead of regex. This would handle all export patterns correctly and is what Next.js effectively does (via SWC AST, not regex). MagicString (already imported at line 41) pairs well with AST-based transforms. AST approach would be ~same LOC but correct for all patterns.
At minimum, bugs 1-3 should be fixed before merging since they produce silently broken output for common patterns.
|
|
||
| const afterEquals = transformed.slice(pos); | ||
| // Check for arrow function with block body | ||
| const arrowMatch = afterEquals.match(/^(?:async\s+)?(?:\([^)]*\)|[a-zA-Z_$]\w*)\s*=>\s*\{/); |
There was a problem hiding this comment.
The arrow regex ^(?:async\s+)?(?:\([^)]*\)|[a-zA-Z_$]\w*)\s*=>\s*\{ doesn't match function expressions (= function() { ... } or = async function name() { ... }), which are common in Next.js codebases (see Next.js's own test suite at test/unit/babel-plugin-next-ssg-transform.test.ts).
When it falls through to the simple assignment branch (line 3726), indexOf(";") finds the first semicolon inside the function body, producing broken output with orphaned code.
Also fails for arrow functions with TS return type annotations like (ctx: Ctx): Promise<Result> => { because [^)]* matches the params, but then : Promise<Result> sits between ) and =>.
| return pos; | ||
| } | ||
|
|
||
| function stripServerExports(code: string): string | null { |
There was a problem hiding this comment.
Missing pattern: export { getServerSideProps } re-export syntax. Next.js's babel-plugin-next-ssg-transform handles this explicitly — it's used in real codebases where the function is declared separately and re-exported.
See: test/unit/babel-plugin-next-ssg-transform.test.ts in the Next.js repo for the full set of export patterns that need handling.
| * point to the opening bracket). Returns the position AFTER the closing bracket. | ||
| * Handles nested brackets, string literals, and comments. | ||
| */ | ||
| function skipBalanced(code: string, pos: number, open: string, close: string): number { |
There was a problem hiding this comment.
skipBalanced doesn't handle regex literals. A } inside a regex like /\{[^}]+\}/ will decrement the brace depth counter, causing premature function body termination. Regex literal detection is hard without a full parser — this is another argument for using parseAst (already imported at line 2) instead of manual scanning.
| if (afterParams < transformed.length) { | ||
| const bodyEnd = skipBalanced(transformed, afterParams, "{", "}"); | ||
| transformed = transformed.slice(0, start) + | ||
| `export function ${name}() { return { props: {} }; }` + |
There was a problem hiding this comment.
Minor: the stub export function getServerSideProps() { return { props: {} }; } means the export name is preserved as a no-op. For getStaticPaths, returning { props: {} } is semantically wrong (should be { paths: [], fallback: false }) — though it likely doesn't matter since these are never called client-side. Next.js takes a different approach: completely removing the export and adding export var __N_SSG=true / export var __N_SSP=true markers instead.
|
Review posted successfully. Here's a summary of what I found: Review Summary for PR #282Verdict: Request Changes The feature itself is necessary — stripping However, I found 5 correctness bugs in the regex-based implementation that produce silently broken client bundles:
Key recommendation: Use Vite's |
Pages Router page modules export getServerSideProps, getStaticProps, and getStaticPaths for server-side use. These functions often import server-only modules (database drivers, fs, etc.) that would break or bloat the client bundle. Added a new Vite transform plugin (vinext:strip-server-exports) that replaces these exports with no-op stubs when the module is being processed for the client environment. The plugin only applies to files under the pages/ directory and skips API routes, _app, _document, and _error files. The transform handles multiple export patterns: - export (async) function declarations - export const arrow functions - export const simple references - TypeScript type annotations in parameters Extracted the core logic into a standalone stripServerExports() function for testability. Added 11 unit tests covering all patterns including edge cases with nested braces, string literals, TypeScript types, and multiple server exports in one file.
Replace the regex-based approach with parseAst (Rollup/acorn) and
MagicString. The AST approach correctly handles all export patterns:
- Function declarations (export function / export async function)
- Variable declarations with any initializer (arrow functions,
function expressions, async named function expressions, simple refs)
- Re-export syntax (export { getServerSideProps })
- Mixed re-exports (export { getServerSideProps, config })
The old regex approach had bugs with function expressions, arrow
functions with TS return type annotations, re-exports, and regex
literals in function bodies. Since this transform runs after Vite's
JSX/TS compilation, parseAst always receives plain JavaScript.
Updated tests to use post-compiled JS (no JSX/TS) matching what the
transform actually receives. Added 6 new test cases for the patterns
that broke the regex approach.
southpolesteve
force-pushed
the
fix/strip-server-exports-from-client
branch
from
30b6819 to
0e53c34
Compare
|
Rewrote What changed:
Bugs fixed:
Test changes:
/bigbonk review this |
There was a problem hiding this comment.
Good rewrite. Switching from regex + skipBalanced to parseAst + MagicString correctly addresses all 5 bugs from the previous review. The new test cases cover each one. All 52 tests pass.
A few things to clean up or be aware of:
Orphaned JSDoc comments (lines 3693–3706)
The skipBalanced and old stripServerExports JSDoc blocks were left behind when their functions were deleted. There are now three consecutive JSDoc comments stacked before the new stripServerExports:
/** Match a Next.js route pattern... */ ← belongs to matchConfigPattern (below)
/** Strip server-only data-fetching... */ ← old stripServerExports docstring
/** Skip past balanced brackets... */ ← old skipBalanced docstring
/** Strip server-only data-fetching... */ ← new stripServerExports docstring
The middle two are dead. Delete them.
Multiple declarators in one export const (low severity)
If someone writes export const getStaticProps = fn, getStaticPaths = fn2; (multiple declarators in a single VariableDeclaration), the loop at line 3742 calls s.overwrite(node.start, node.end, ...) on the first match, then tries the same range again on the second match. MagicString throws on overlapping overwrites to the same range, so this would error at transform time rather than silently produce bad output.
This is extremely unlikely in practice (ESLint/Prettier enforce one-per-line, and the pattern is uncommon), but if you want to be defensive, accumulate all matched declarator names from the loop and emit a single overwrite after:
const names = decl.declarations
.filter(d => d.id?.type === "Identifier" && SERVER_EXPORTS.has(d.id.name))
.map(d => d.id.name);
if (names.length > 0) {
s.overwrite(node.start, node.end, names.map(n => `export const ${n} = undefined;`).join("\n"));
changed = true;
}This also handles the case where a VariableDeclaration mixes server and non-server exports (export const getStaticProps = fn, config = 'edge';), which the current code silently drops config. Same fix covers both.
Sourcemaps
The transform returns map: null (line 2960). Since MagicString can generate accurate sourcemaps with s.generateMap(), it'd be worth returning one. map: null tells Vite "no position changes" which isn't true after overwrites. Not a correctness issue but hurts debugging.
Missing negative test for export { name } from './other'
The !node.source guard at line 3753 correctly skips re-exports from other modules (export { getServerSideProps } from './data'). Worth adding a test to pin this behavior — you don't want to accidentally strip re-exports that just pass through the name.
Test quality
The test updates are well done. Removing JSX/TS from test inputs to match what the transform actually receives (post-esbuild) is the right call. The "TypeScript type annotations" test was correctly removed since that's moot with AST parsing of compiled output. The comment linking to Next.js's babel-plugin-next-ssg-transform.test.ts is helpful for future reference.
|
Review posted. Here's the summary: Verdict: Approve The rewrite from regex to I flagged 4 items in the review:
|
1 participant
Summary
vinext:strip-server-exports) that removesgetServerSideProps,getStaticProps, andgetStaticPathsexports from page modules in the client bundlefs, etc.) that would break or bloat the client bundle. Next.js does this via an SWC transform; this adds the equivalent for vinext.export function getServerSideProps() { return { props: {} }; }orexport const ... = undefined;) so the bundler can tree-shake their import chainspages/directory. API routes,_app,_document, and_errorare excluded.export function,export async function,export constarrow functions,export constsimple references, and TypeScript type annotations in parametersImplementation
The core logic is in a standalone
stripServerExports()function (exported as_stripServerExportsfor testing). It uses askipBalanced()helper for bracket-aware scanning that correctly handles nested braces, string/template literals, line/block comments, and TypeScript type annotations.Test plan
11 unit tests added to
build-optimization.test.tscovering:export async function getServerSidePropsstrippingexport function getStaticPropsstrippingexport async function getStaticPaths+getStaticPropstogetherexport constarrow function strippingexport constsimple reference strippingFull test suite (2142 tests) passes. Existing pages-router integration tests confirm no regressions.