Conversation
a92bbc1 to
8bb7b70
Compare
commit: |
|
|
/bigbonk review |
There was a problem hiding this comment.
Clean abstraction for the Vite 7/8 bundler config split. All CI green, deploy previews healthy. A few observations:
What looks good
- The
getBuildBundlerOptions/withBuildBundlerOptions/getClientOutputConfigForVitetrio is a clean seam — centralizes the version branching so call sites stay readable. getBuildBundlerOptionscorrectly prefersrolldownOptionsoverrollupOptionswhen reading user config, so users supplying the legacy key on Vite 8 still get theironwarnpicked up.- The
clientCodeSplittingConfigmapping from Rollup'smanualChunks+experimentalMinChunkSizeto Rolldown'scodeSplitting.groups[].name+codeSplitting.minSizelooks correct. - Test helpers (
getBuildBundlerOptions,getEnvBuildBundlerOptions) make assertions version-agnostic without losing coverage — theif (output.codeSplitting)branch in the chunk sizing test is a nice touch. - The "preserves user-supplied
build.rollupOptions.onwarn" test passesrollupOptionsas mock input, which validates the legacy fallback path ingetBuildBundlerOptions. Good.
Minor observations (non-blocking)
-
Duplicate
withBuildBundlerOptionsincli.ts: The CLI defines its own local closure(bundlerOptions: Record<string, unknown>) => ...instead of importing the one fromindex.ts. The signatures differ slightly (Record<string, unknown>vsVinextBuildBundlerOptions), so I understand why — the CLI doesn't have access to the Vite type forVinextBuildBundlerOptionsat that point. Fine for now, but if this grows, consider exporting a loosely-typed version fromindex.ts. -
clientCodeSplittingConfig.groups[].namereturnsnull: Thenamefunction returnsclientManualChunks(moduleId) ?? null. I assume Rolldown'scodeSplitting.groups[].nametreatsnullas "don't assign a custom name" (fall through to default splitting). Worth a quick sanity check that Rolldown doesn't treatnulldifferently fromundefinedhere, but given all E2E and deploy previews pass, this is clearly working. -
No test for user-supplied
build.rolldownOptions.onwarn: The existing test passes{ build: { rollupOptions: { onwarn } } }which validates the legacy path. A parallel test with{ build: { rolldownOptions: { onwarn } } }would cover the Vite 8 path directly — not blocking sincegetBuildBundlerOptionsprefersrolldownOptionsfirst, so the logic is exercised, but it would be a nice addition for completeness.
Everything else — version bumps, lock file cleanup, peer dep range — looks correct. All 30 CI checks pass including full E2E suites and deploy previews.
|
Review posted and PR approved. Here's a summary of the review: Approved PR #599 — The PR cleanly abstracts the Vite 7/8 bundler config divergence ( Three non-blocking observations were noted:
|
Summary
rolldownOptionson Vite 8 while still honoring user-supplied legacybuild.rollupOptionscodeSplittingon Vite 8 and update tests to accept both config shapesTesting