fix(benchmarks): run SSR setup imports natively#7579
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBench scripts now run vitest benches under NODE_ENV=production. React, Solid, and Vue SSR benchmarks compute absolute module URLs from import.meta.url and eagerly import the server bundle default (StartRequestHandler) at module scope, removing setup/teardown and suite hooks. ChangesSSR Benchmark Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit b57ae87
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview1 package(s) bumped directly, 4 bumped as dependents. 🟩 Patch bumps
|
Bundle Size Benchmarks
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
Merging this PR will improve performance by 82.36%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | ssr request loop (react) |
327.9 ms | 84.2 ms | ×3.9 |
| ⚡ | ssr request loop (vue) |
481.7 ms | 289.6 ms | +66.34% |
| 👁 | client-side navigation loop (react) |
55.8 ms | 59.6 ms | -6.35% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix-ssr-codspeed-benchmark (b57ae87) with main (4a93cff)
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
benchmarks/ssr/react/speed.bench.ts (1)
1-45: ⚖️ Poor tradeoffConsider extracting the shared benchmark pattern.
All three SSR benchmark files (React, Solid, Vue) follow an identical structure, differing only in the benchmark seed value and framework label. While this duplication ensures each benchmark remains self-contained, extracting the common lazy-loading, setup/teardown, and benchmarking logic into a shared utility could improve maintainability if this pattern evolves.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/ssr/react/speed.bench.ts` around lines 1 - 45, Extract the repeated lazy-loading and benchmark wiring into a shared helper (e.g., a factory like createSsrBenchmark) in bench-utils: move the logic from loadHandler, setup, teardown, runBenchmark and the import of appModuleUrl into that helper and parameterize it by module URL and seed (benchmarkSeed) so each framework file only calls the helper with its specific appModuleUrl and seed and supplies the framework label to describe/bench; update the current file to import and invoke that helper, removing duplicated functions and keeping only the small per-framework constants and the bench() call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@benchmarks/ssr/react/speed.bench.ts`:
- Around line 1-45: Extract the repeated lazy-loading and benchmark wiring into
a shared helper (e.g., a factory like createSsrBenchmark) in bench-utils: move
the logic from loadHandler, setup, teardown, runBenchmark and the import of
appModuleUrl into that helper and parameterize it by module URL and seed
(benchmarkSeed) so each framework file only calls the helper with its specific
appModuleUrl and seed and supplies the framework label to describe/bench; update
the current file to import and invoke that helper, removing duplicated functions
and keeping only the small per-framework constants and the bench() call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9779c565-62a4-4dd9-a795-0c2507b177d9
📒 Files selected for processing (3)
benchmarks/ssr/react/speed.bench.tsbenchmarks/ssr/solid/speed.bench.tsbenchmarks/ssr/vue/speed.bench.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
benchmarks/ssr/react/speed.bench.ts (1)
8-12: 💤 Low valueOptional: Consider extracting the common pattern across React/Solid/Vue benchmarks.
All three SSR benchmark files (React, Solid, Vue) follow an identical structure: compute
appModuleUrl, top-level dynamic import with/*@vite-ignore*/, destructure and type-assert the handler, then callrunSsrRequestLoopdirectly in the benchmark body. The only differences are the seed values and benchmark names.While the current duplication is minimal and benchmark independence may be preferred, extracting a shared factory helper could reduce maintenance overhead if the pattern evolves.
Example shared factory approach
// bench-utils.ts or new file export function createSsrBenchmark( framework: string, seed: number, ) { const appModuleUrl = new URL(`./dist/server/server.js`, import.meta.url).href return { appModuleUrl, seed, async loadHandler() { const { default: handler } = (await import( /* `@vite-ignore` */ appModuleUrl )) as { default: StartRequestHandler } return handler }, } }Then in each benchmark file:
const { seed, loadHandler } = createSsrBenchmark('react', 0xdecafbad) const handler = await loadHandler()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/ssr/react/speed.bench.ts` around lines 8 - 12, Extract the duplicated SSR benchmark pattern into a shared factory (e.g., createSsrBenchmark) that returns appModuleUrl, seed, and a loadHandler() which performs the top-level dynamic import using /* `@vite-ignore` */ and type-asserts the default as StartRequestHandler; then update each framework benchmark (React/Solid/Vue) to call createSsrBenchmark(...), await loadHandler() to get handler, and pass that handler into runSsrRequestLoop, keeping only framework-specific seed and benchmark name in each file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@benchmarks/ssr/react/speed.bench.ts`:
- Around line 8-12: Extract the duplicated SSR benchmark pattern into a shared
factory (e.g., createSsrBenchmark) that returns appModuleUrl, seed, and a
loadHandler() which performs the top-level dynamic import using /* `@vite-ignore`
*/ and type-asserts the default as StartRequestHandler; then update each
framework benchmark (React/Solid/Vue) to call createSsrBenchmark(...), await
loadHandler() to get handler, and pass that handler into runSsrRequestLoop,
keeping only framework-specific seed and benchmark name in each file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d5d96e60-d86e-4ca4-b18c-6a63d5fbcfe0
📒 Files selected for processing (3)
benchmarks/ssr/react/speed.bench.tsbenchmarks/ssr/solid/speed.bench.tsbenchmarks/ssr/vue/speed.bench.ts
2 participants
Summary
Notes
/* @vite-ignore */in import analysis and recommends it in its dynamic import warning for intentionally opaque imports. It suppresses analysis/warnings for this Node-only native import path.Test Plan
Summary by CodeRabbit
Chores
Refactor