fix(vite-lib-config)!: migrate to vite 8 and its consumers to match - #127
Conversation
vite@8 replaces rollup/esbuild with rolldown/oxc, so the shared viteConfig() needs two adjustments in vite.mts: rename build.rollupOptions to build.rolldownOptions in both the static config and the per-entry override, and drop build.rollupOptions.output.importAttributesKey, which rolldown's OutputOptions has no equivalent for. Bumps vite from ^7.1.9 to ^8.0.0 (the oldest working Vite 8 release, not npm latest, per this repository's version-floor policy) in vite-lib-config plus its three consumers (example-cli, example-lib, sea-builder), since they run viteConfig() under their own locally installed vite binary and would fail to resolve the renamed rolldownOptions.input entry point under vite@7 if bumped separately. also bumps vite-plugin-dts from ^4.5.4 to ^5.0.1 (^5.0.0 itself bundles a broken unplugin-dts@1.0.0 that fails vite-lib-config's typedoc-based build:doc step). verified end to end: pnpm install --frozen-lockfile, lint, build (including doc generation), and test (test:ts + test:vitest, 80/80) all pass under the bumped dependency set. BREAKING CHANGE: vite is now ^8.0.0 (was ^7.1.9) in vite-lib-config's runtime dependencies, and vite-plugin-dts is now ^5.0.1 (was ^4.5.4). consumers of the published viteConfig() that pass through build.rollupOptions.output.importAttributesKey must remove it -- rolldown has no equivalent option. build.rollupOptions itself still works under Vite 8's backward-compat alias but is deprecated in favor of build.rolldownOptions, so this package migrates proactively to avoid a second forced change later. Refs #121
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 45 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR upgrades internal packages to Vite 8 and ChangesVite 8 migration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This pull request migrates the shared @kurone-kito/vite-lib-config Vite configuration factory and all in-repo consumers to Vite 8, aligning the workspace with Vite’s Rolldown-based build configuration surface and updating related tooling and documentation.
Changes:
- Updated the shared Vite config to use
build.rolldownOptions(and removed the unsupportedimportAttributesKeyoutput option). - Bumped
viteto^8.0.0acrossvite-lib-config,sea-builder,example-cli, andexample-lib, and bumpedvite-plugin-dtsto^5.0.1invite-lib-config. - Regenerated
pnpm-lock.yamland added corresponding changelog entries for the published packages.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Lockfile regeneration reflecting Vite 8 / Rolldown and updated dependency graph. |
| packages/vite-lib-config/src/vite.mts | Migrates config factory from rollupOptions to rolldownOptions and removes importAttributesKey. |
| packages/vite-lib-config/package.json | Bumps vite to ^8.0.0 and vite-plugin-dts to ^5.0.1. |
| packages/vite-lib-config/CHANGELOG.md | Documents the breaking migration and removal of importAttributesKey. |
| packages/sea-builder/package.json | Bumps devDependency vite to ^8.0.0. |
| packages/sea-builder/CHANGELOG.md | Notes the Vite devDependency bump. |
| packages/example-lib/package.json | Bumps devDependency vite to ^8.0.0. |
| packages/example-cli/package.json | Bumps devDependency vite to ^8.0.0. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Rejected — chatgpt-codex-connector[bot] did not review HEAD 727d50d (Codex usage limits for code reviews reached); this is not a completed review (source: #issuecomment-5265544709) |
|
Rejected — coderabbitai[bot] did not review HEAD 727d50d (review limit reached / rate limited); this is not a completed review (source: #issuecomment-5265545650) |
adds an assertion that viteConfig()'s emitted build config sets build.rolldownOptions.input for the entry point, so a future regression reverting the rollupOptions -> rolldownOptions rename (#121) would fail this test instead of passing silently -- the existing suite only checked build.lib/outDir/plugins, none of which touch the renamed key. confirmed empirically (via a standalone probe script, not asserted in this test) that vite@8's own mergeConfig mirrors rolldownOptions into a build.rollupOptions alias in the merged output for backward compatibility, so a negative "not.toHaveProperty('build.rollupOptions')" assertion would be factually wrong -- this test only asserts the positive rolldownOptions.input value.
the E10 critique pass on the prior commit (d0b7427) found the test added there did not actually catch the regression it claimed to guard against: probed vite@8's real mergeConfig and confirmed it bidirectionally aliases build.rollupOptions and build.rolldownOptions in its merged output, so a runtime-output assertion cannot tell which key vite.mts declares -- reverting the rename would still pass the prior test. replace it with a source-text assertion instead (read vite.mts's raw content and check for rolldownOptions presence / rollupOptions and importAttributesKey absence), which genuinely fails when reverted -- verified by simulating the revert against a throwaway copy of the string content, not the tracked file.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (1)
packages/vite-lib-config/CHANGELOG.md:38
- The changelog entry references
build.rollupOptions.output.importAttributesKeyeven though the config has been migrated tobuild.rolldownOptions, and the inline-code snippet forassert { type: ... }is split across a line break (Markdown inline code cannot contain newlines), which will render incorrectly.
`build.rollupOptions.output.importAttributesKey` — Rolldown's
`OutputOptions` has no equivalent option, so downstream consumers of
the published `viteConfig()` that relied on it to control emitted
import-attribute syntax (`with { type: ... }` / `assert { type: ...
}`) must drop that reliance (#121).
github copilot's second review of this pr (at 94b3e6d) surfaced a suppressed comment: the #121 changelog entry's inline-code span for assert { type: ... } wrapped across a line break, which commonmark renders with the newline collapsed to a space plus the continuation line's own leading indentation preserved literally, producing extra whitespace inside the rendered code span. rewords the sentence so every inline-code span opens and closes on the same source line.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (2)
packages/vite-lib-config/src/vite.spec.mts:160
- The source-text assertion is overly broad:
not.toContain('rollupOptions')will fail ifvite.mtsever mentions the term in a comment/string, even if the config correctly usesrolldownOptions. It’s less brittle to assert on property-key syntax (e.g.,rollupOptions:/rolldownOptions:) rather than any substring occurrence.
expect(source).toContain('rolldownOptions');
expect(source).not.toContain('rollupOptions');
expect(source).not.toContain('importAttributesKey');
packages/vite-lib-config/CHANGELOG.md:34
- This entry says the removed option was
build.rollupOptions.output.importAttributesKey, but the breaking change also renamesrollupOptionstorolldownOptions. Updating this reference makes the changelog clearer about which key exists in the new version vs the old one.
the `build.rollupOptions.output.importAttributesKey` option —
github copilot's third review of this pr (at 1070cee) surfaced two suppressed comments, both accepted and fixed here: - vite.spec.mts's source-text regression test matched any bare substring occurrence of rollupOptions/importAttributesKey, which would false-fail on a future prose mention of the old key names even with a correctly migrated config. match property-key syntax (a trailing colon) instead -- copilot's own suggestion, and the same brittleness an earlier local e10 critique pass had already flagged as low severity and left as-is; two independent findings on the same spot outweighed that earlier call. verified the tightened assertion still fails against a simulated pre-migration revert. - the changelog's #121 entry named the removed option as build.rollupOptions.output.importAttributesKey without clarifying that path only existed under the old key name pre-rename; reworded to say so explicitly.
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/vite-lib-config/src/vite.spec.mts (1)
145-162: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the active configuration declarations.
The test only checks for source substrings. It can pass when
rolldownOptionsappears in a comment or unused code while the active configuration is missing the migrated properties. Match theoutputandinputdeclarations, or parse the source, so the regression guard checks the active configuration.The PR objective states that this test covers the Rolldown configuration migration.
Proposed assertion
- expect(source).toContain('rolldownOptions'); + expect(source).toMatch(/rolldownOptions:\s*\{\s*output:/); + expect(source).toMatch(/rolldownOptions:\s*\{\s*input:\s*entry\s*\}/);🤖 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 `@packages/vite-lib-config/src/vite.spec.mts` around lines 145 - 162, Update the “build configuration source” test to validate the active configuration rather than arbitrary source substrings: match or parse the `build.rolldownOptions` declaration and assert its `output` and `input` properties, while preserving checks that `rollupOptions` and `importAttributesKey` are absent from the migrated configuration. Use the existing `readFile`-based test and its visible configuration symbols.
🤖 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 `@packages/vite-lib-config/src/vite.spec.mts`:
- Around line 145-162: Update the “build configuration source” test to validate
the active configuration rather than arbitrary source substrings: match or parse
the `build.rolldownOptions` declaration and assert its `output` and `input`
properties, while preserving checks that `rollupOptions` and
`importAttributesKey` are absent from the migrated configuration. Use the
existing `readFile`-based test and its visible configuration symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 663e9af3-8fa8-41ac-be80-7a4804f8eebd
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
packages/example-cli/package.jsonpackages/example-lib/package.jsonpackages/sea-builder/CHANGELOG.mdpackages/sea-builder/package.jsonpackages/vite-lib-config/CHANGELOG.mdpackages/vite-lib-config/package.jsonpackages/vite-lib-config/src/vite.mtspackages/vite-lib-config/src/vite.spec.mts
coderabbit's completed review of this pr (at 1070cee) raised a nitpick: the source-text regression test still only checked for a property-key substring, which can pass when rolldownOptions appears in a comment or unused code while the active configuration is missing the migrated properties. adopt its suggested fix: match the actual declaration shapes (staticConfig's rolldownOptions: { output: ... } nesting and innerCreateConfig's rolldownOptions: { input: entry } override) with a regex, instead of a bare property-key substring check. verified this still fails against a simulated pre-migration revert, and passes against the current source.
|
Accepted — fixed in a58ce1a: replaced the property-key substring check in vite.spec.mts with a regex matching the actual rolldownOptions declaration shapes (staticConfig's |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
F4 Cleanup Evidence (server-side fallback via
|
Summary
viteConfig()/vitestConfig()factory inpackages/vite-lib-config/src/vite.mtsfrom Rollup/esbuild to Vite 8's Rolldown-based bundler: renamebuild.rollupOptionstobuild.rolldownOptionsin bothstaticConfigand the per-entry override, and deletebuild.rollupOptions.output.importAttributesKey(Rolldown'sOutputOptionshas no equivalent option).vitefrom^7.1.9to^8.0.0(the oldest stable Vite 8 release, not npmlatest— matching this repository's version-floor convention) inpackages/vite-lib-config,packages/example-cli,packages/example-lib, andpackages/sea-builder. All three consumers runviteConfig()under their own locally installedvite, so the bump had to land for all four packages in the same commit — a split would leaveexample-cli/sea-builder(bin-mode builds, entries supplied only viarolldownOptions.input) unable to resolve their build entry under a still-vite@7binary.vite-plugin-dtsfrom^4.5.4to^5.0.1inpackages/vite-lib-config(^5.0.0itself bundles a brokenunplugin-dts@1.0.0that fails thetypedoc-basedbuild:docstep withTS2307/TS2882errors against optional peer bundler types;5.0.1bundles a fixedunplugin-dts@1.0.1).pnpm-lock.yamlaccordingly.## [Unreleased]/### Changedentries (marked Breaking invite-lib-config) topackages/vite-lib-config/CHANGELOG.mdandpackages/sea-builder/CHANGELOG.md.example-cli/example-libare unpublished with noCHANGELOG.md.Closes #121
Verification
Ran locally on this branch, all green:
pnpm install --frozen-lockfile— clean install against the regenerated lockfile.pnpm run lint:fixthenpnpm run lint— clean, no fixes needed.pnpm run build— all five packages build undervite@8.2.1(resolved from^8.0.0), includingvite-lib-config'stypedoc-based doc-generation step (log prefix confirmsvite-plugin-dts@5.0.3'sunplugin-dtsrewrite is active).pnpm run test—test:ts(workspace-widetsc -p tsconfig.test.json --noEmit) passes with no new errors;test:vitestpasses 22/22 files, 80/80 tests.Background
Independently verified the issue's floor-check claims before implementing (not just trusting the issue body):
npm view vite versionsconfirms8.0.0is a real stable release (no non-beta release between7.3.6and8.0.0; dist-tags showprevious: 7.3.6,latest: 8.2.1).npm view vite-plugin-dts@5.0.0 dependencies/@5.0.1 dependenciesconfirm theunplugin-dts@1.0.0→1.0.1bisection point the issue's floor check describes.vite-plugin-dtsis still published under the same package name at5.0.1— only the upstream GitHub repo moved toqmhc/unplugin-dts; no dependency-name change was needed, only the version pin.This is a breaking change for downstream consumers of the published
@kurone-kito/vite-lib-configviteConfig()factory who relied onimportAttributesKeyto control emitted import-attribute syntax — see theBREAKING CHANGE:footer on the migration commit and the CHANGELOG entries for details.Summary by CodeRabbit
Chores
Documentation
Tests