-
Notifications
You must be signed in to change notification settings - Fork 10
feat: add vite environments api compatibility #236
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.
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
Conversation
similar to what cloudflare vite plugin does here https://github.com/cloudflare/workers-sdk/blob/24765d1c1e5cf8d48ea719440a6f74a6b5d90402/packages/vite-plugin-cloudflare/src/plugins/config.ts#L85 we do it in buildApp instead of config which runs later, but it retains backwards compatibility with the legacy vite apis
🦋 Changeset detectedLatest commit: 4789c74 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds build orchestration and SSR-aware behavior to the Vite plugin: introduces a pre-plugin Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/index.ts (2)
432-435: Potential duplication with configEnvironment hook.This logic setting
emptyOutDir = falsefor non-SSR builds is duplicated in theconfigEnvironmenthook (lines 531-534). While this may be intentional for backward compatibility with legacy Vite (before environments API), it could lead to confusion about which hook controls this setting.Consider adding a comment explaining that this is for legacy Vite compatibility, or refactoring to centralize the emptyOutDir logic:
if (!isSSRBuild) { + // Legacy Vite: prevent client build from clearing SSR output config.build ??= {}; config.build.emptyOutDir = false; }
493-537: Consider extracting duplicated configuration logic.The
configEnvironmenthook duplicates substantial logic from theconfighook:
- noExternal handling (lines 496-508 vs 395-408)
- chunkFileNames configuration (lines 513-521 vs 410-419)
- esmExternals setup (lines 522-530 vs 421-430)
While this duplication supports both legacy and environments API modes, it increases maintenance burden.
Consider extracting the shared logic into helper functions:
function configureNoExternal(noExternal: any): any { if (noExternal === true) return noExternal; const noExternalReg = /\.marko$/; if (noExternal) { return Array.isArray(noExternal) ? [...noExternal, noExternalReg] : [noExternal, noExternalReg]; } return noExternalReg; } function configureSSRBuildOptions(options: any, rootResolveFile: string) { options.build ??= {}; if (!options.build?.rollupOptions?.output) { options.build.rollupOptions ??= {}; options.build.rollupOptions.output = { chunkFileNames: `[name]-[hash].js`, }; } if (!options.build?.commonjsOptions?.esmExternals) { options.build.commonjsOptions ??= {}; options.build.commonjsOptions.esmExternals = (id) => !isCJSModule(id, rootResolveFile); } }Then use these helpers in both hooks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/index.ts(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/index.ts (1)
src/resolve.ts (1)
isCJSModule(10-51)
🔇 Additional comments (4)
src/index.ts (4)
603-635: LGTM! Proper dual-mode SSR detection.The condition correctly checks both
isSSRBuild(legacy mode) andthis?.environment?.name === "ssr"(environments API mode), ensuring backward compatibility while supporting the new Vite environments API.
636-650: LGTM! Correct dual-mode non-SSR detection.The negated condition properly identifies non-SSR builds in both legacy mode (
!isSSRBuild) and environments API mode (this?.environment?.name !== "ssr"), correctly loading server assets during client builds.
983-987: LGTM! Consistent plugin sharing configuration.Adding
sharedDuringBuild: trueto the post plugin matches the pre plugin configuration, correctly enabling plugin instance sharing during environment-based builds as described in the PR objectives.
1000-1040: LGTM! Consistent dual-mode SSR detection in bundle generation.The condition properly checks both modes for SSR context when handling server manifests and assets during bundle generation, maintaining consistency with the SSR detection pattern used throughout the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/frank-wings-share.md (1)
1-5: Minor: Capitalize "Vite" in changeset description.The changeset structure is valid and the minor version bump is appropriate for this feature addition. However, the description should capitalize "Vite" for brand consistency.
--- "@marko/vite": minor --- -compatibility with the vite environments api +Adds vite environments API compatibilityThis slight rewording also clarifies the purpose and follows common changelog conventions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/index.ts (1)
432-435: emptyOutDir and env‑specific config are consistent, but SSR logic is duplicatedThe combination of:
- Line 432–435: forcing
config.build.emptyOutDir = falsefor non‑SSR linked builds, and- Lines 493–539: per‑environment
configEnvironmentthat setsemptyOutDir = trueforssrandfalsefor other envs when linked, while mirroring the.markonoExternal,chunkFileNames, andesmExternalslogicproduces the intended behavior for both legacy linked builds and the environments API. The main downside is that the SSR config (noExternal/chunkFileNames/esmExternals/emptyOutDir) is now duplicated between
configandconfigEnvironment, which increases drift risk if one side is tweaked later.Consider extracting a small helper (e.g.
applySSRBuildConfig(targetBuild, targetResolve, rootResolveFile)or similar) that bothconfigandconfigEnvironment("ssr", ...)call, so changes stay in sync while keeping the env‑specificemptyOutDiroverride. This is non‑blocking but would make future maintenance safer.Also applies to: 493-539
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/index.ts(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mpeg
Repo: marko-js/vite PR: 236
File: src/index.ts:207-222
Timestamp: 2025-11-24T13:52:04.055Z
Learning: In Vite's buildApp hook (available in Vite 6+), the builder parameter has a config property that can contain builder.buildApp. The property path builder.config.builder.buildApp is valid for checking if a user has provided a custom buildApp override in their Vite configuration to control build order and avoid conflicts between multiple plugins.
📚 Learning: 2025-11-24T13:52:04.055Z
Learnt from: mpeg
Repo: marko-js/vite PR: 236
File: src/index.ts:207-222
Timestamp: 2025-11-24T13:52:04.055Z
Learning: In Vite's buildApp hook (available in Vite 6+), the builder parameter has a config property that can contain builder.buildApp. The property path builder.config.builder.buildApp is valid for checking if a user has provided a custom buildApp override in their Vite configuration to control build order and avoid conflicts between multiple plugins.
Applied to files:
src/index.ts
🧬 Code graph analysis (1)
src/index.ts (2)
src/__tests__/fixtures/isomorphic-runtime-base-path/test.config.ts (2)
ssr(17-17)options(25-27)src/resolve.ts (1)
isCJSModule(10-51)
🔇 Additional comments (3)
src/index.ts (3)
206-222: buildApp hook wiring and override behavior look correctThe
sharedDuringBuild: trueflag plusbuildApp(builder)implementation align with Vite’s environments API: you only take over when bothssrandclientenvironments exist, preserve legacy behavior when they don’t, and correctly defer to a user‑providedbuilder.config.builder.buildAppto avoid double orchestration. The linked‑onlyssrbuild followed by an unconditional client build matches the documented “SSR then client for linked builds” semantics. Based on learnings, thebuilder.config.builder.buildAppoverride check is the right shape.
607-635: SSR environment detection via this.environment is coherent across hooksUsing
isSSRBuild || this?.environment?.name === "ssr"inoptions(Line 607) andgenerateBundle(Line 1013), and the complementary guard inbuildStart(Lines 639–645), cleanly unifies legacy CLI SSR builds (isSSRBuildfromconfig.build.ssr) with the new environments API (this.environment.name === "ssr"). This ensures:
- The SSR manifest is initialized exactly once per SSR environment.
- Browser/client builds (legacy or env‑based) correctly consume the persisted SSR manifest and eagerly
loadSSR assets.- The same SSR vs browser branch is taken in
generateBundleregardless of whether SSR is driven by--ssror by the env API.The optional chaining on
this?.environmentis conservative but harmless given plugin hook contexts. Overall the flow looks correct and consistent with the newbuildApp/configEnvironmentbehavior.Also applies to: 639-645, 1013-1043
986-990: Post plugin sharedDuringBuild flag matches pre plugin behaviorMarking the
marko-vite:postplugin assharedDuringBuild: trueis important so its state (notably access toserverManifestwritten by the pre plugin and shared closure state) is reused across SSR/client environment builds within a single app build. This keepsgenerateBundle’s SSR and browser passes aligned with the pre plugin’sbuildApporchestration and env‑awareoptionslogic. Comment tweak is fine as well.
|
I've fixed the unnecessary optional chaining, since builder will always be defined when the buildApp hook is called, and Also added a linked guard around the emptyOutDir default, so it only defaults to not emptying it on client builds when part of a linked build. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/index.ts (1)
493-539: Consider extracting shared SSR build config into a reusable helper.The
configEnvironment("ssr", options)branch largely duplicates the SSR setup done earlier inconfig(...)(adding.markotonoExternal, overridingchunkFileNames, and wiringcommonjsOptions.esmExternals). To avoid the two paths drifting over time, you could factor this into a small helper (e.g.applySSRBuildDefaults(configLike)andapplySSRNoExternal(configLike)), and call it from bothconfigandconfigEnvironment. This keeps legacy--ssrand environment‑API SSR behavior in lockstep.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/index.ts(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mpeg
Repo: marko-js/vite PR: 236
File: src/index.ts:207-222
Timestamp: 2025-11-24T13:52:04.055Z
Learning: In Vite's buildApp hook (available in Vite 6+), the builder parameter has a config property that can contain builder.buildApp. The property path builder.config.builder.buildApp is valid for checking if a user has provided a custom buildApp override in their Vite configuration to control build order and avoid conflicts between multiple plugins.
📚 Learning: 2025-11-24T13:52:04.055Z
Learnt from: mpeg
Repo: marko-js/vite PR: 236
File: src/index.ts:207-222
Timestamp: 2025-11-24T13:52:04.055Z
Learning: In Vite's buildApp hook (available in Vite 6+), the builder parameter has a config property that can contain builder.buildApp. The property path builder.config.builder.buildApp is valid for checking if a user has provided a custom buildApp override in their Vite configuration to control build order and avoid conflicts between multiple plugins.
Applied to files:
src/index.ts
🧬 Code graph analysis (1)
src/index.ts (1)
src/resolve.ts (1)
isCJSModule(10-51)
🔇 Additional comments (3)
src/index.ts (3)
432-435: Double‑check the impact of forcingemptyOutDir = falsefor all non‑SSR linked builds.With
linkeddefaulting totrue, this will also apply to plain browser builds (e.g.vite buildwithout a preceding SSR build), meaningoutDiris no longer cleaned by default and old artifacts may accumulate. That’s probably fine for the linked SSR+client workflow, but it is a behavior change vs Vite defaults; worth confirming this is intentional and reflected in docs/expectations.
605-637: Unified SSR detection inoptions/buildStartlooks correct.Using
isSSRBuild || this.environment?.name === "ssr"inoptionsand the mirrored negation inbuildStartensuresserverManifestis created for SSR builds (legacy and environment‑API) and loaded only for the corresponding non‑SSR/browser build. Thelinked && isBuildguards also preventserverManifest!from being touched in dev/test. This is a clean way to support both code paths without changing the existing manifest flow.Also applies to: 639-645
986-990: Post‑pluginsharedDuringBuildand SSR branch gating align with the pre‑plugin changes.Marking the post plugin as
sharedDuringBuild: truematches the pre plugin and ensures a single sharedserverManifest/store is used across environments. The updated conditionif (isSSRBuild || this.environment?.name === "ssr")ingenerateBundlecorrectly confines manifest collection and persistence to the server build for both legacy--ssrand the newssrenvironment, while the else‑branch continues to handle the browser manifest case. No issues spotted here.Also applies to: 1013-1043
isSSRBuildflag, we add a matchingthis.environmentcheck for compatibility with the vite env apisharedDuringBuildflag on both plugins, so one instance is shared on an app buildconfigEnvironmenthook, a bit of duplication here with theconfighook for ssr builds, but intent is clearer this waybuildApphook which by default builds in order, first ssr then client env (when doing a linked build). If both envs are not set it assumes vite is on legacy mode and does nothing to avoid extra builds.buildAppcan be overriden by the end user throughbuilder.buildAppin the vite config, this matches the behaviour of other plugins such as the cloudflare vite pluginOn Vite 6 and under, the new hooks are ignored and do nothing.
A minimal vite config to work with the new APIs would look like this
and build with
vite build --appfor the legacy APIs, you can still specify the entry point on the --ssr command like
vite build --ssr src/entry.ts && vite build