wip: vapor mode - #2509
Conversation
✅ Deploy Preview for vue-router ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
commit: |
|
Note for myself for refactoring and simplification: Playground |
|
Is this already compatible with “3.6.0-alpha.1”? |
You can try it in vue@3.6.0-alpha.2 |
|
The automatic global registration of the virtual DOM |
|
FYI this is planned after #2415 which will also allow to register a custom RouterView and RouterLink |
|
Is this a last blocker of the epic of embedding Vapor into the core? |
|
No |
|
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:
📝 WalkthroughWalkthroughAdds Vapor-compatible RouterLink and RouterView components, test helpers and large Vitest suites, updates package manifests and playground files to use Vapor wiring, and prevents global $router/$route injection when app.vapor is true. ChangesVapor router feature
Sequence Diagram(s)sequenceDiagram
participant User
participant VaporRouterLink
participant Router
participant VaporRouterView
participant Component
User->>VaporRouterLink: click (on link)
VaporRouterLink->>Router: navigate (push/replace)
Router->>Router: update current location / matched records
Router->>VaporRouterView: notify view depth / matchedRoute change
VaporRouterView->>Component: instantiate/render matched component (with props)
Component-->>VaporRouterView: mounted/rendered
VaporRouterView-->>User: DOM updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2509 +/- ##
==========================================
+ Coverage 86.05% 86.17% +0.11%
==========================================
Files 74 76 +2
Lines 5896 5973 +77
Branches 1874 1898 +24
==========================================
+ Hits 5074 5147 +73
- Misses 726 728 +2
- Partials 96 98 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Is there any progress? |
|
It works, You can try it: |
|
Will the next step be to make this available in a beta version tag or will it simply be a new release when vue 3.6 is out of beta? |
|
When using a vapor only app with |
|
The experimental (and future) router already stopped registering those components because of that |
| resolve: { | ||
| alias: { | ||
| // cjs does not export vapor runtime, use esm instead. | ||
| vue: 'vue/dist/vue.esm-bundler.js', |
There was a problem hiding this comment.
did you manage to add any test? That would be great to iterate on details
There was a problem hiding this comment.
Pull request overview
This PR adds experimental Vapor mode support to vue-router by introducing two new components: VaporRouterView and VaporRouterLink. The implementation conditionally disables global properties ($router and $route) in Vapor mode and includes comprehensive test coverage for both components.
Changes:
- Added Vapor-compatible
VaporRouterViewandVaporRouterLinkcomponents with full functionality - Conditionally disabled global properties injection for Vapor apps
- Updated Vue dependency to a pre-release version that includes Vapor support
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/router/src/VaporRouterView.ts | New Vapor-compatible RouterView component implementation |
| packages/router/src/VaporRouterLink.ts | New Vapor-compatible RouterLink component implementation |
| packages/router/src/router.ts | Added conditional check to skip global properties in Vapor mode |
| packages/router/src/RouterLink.ts | Exported getLinkClass function for reuse in VaporRouterLink |
| packages/router/src/index.ts | Exported new Vapor components |
| packages/router/tests/VaporRouterView.spec.ts | Comprehensive test suite for VaporRouterView |
| packages/router/tests/VaporRouterLink.spec.ts | Comprehensive test suite for VaporRouterLink |
| packages/router/tests/mount.ts | Added test mounting utilities for Vapor components |
| packages/router/package.json | Updated dependencies to pre-release versions |
| packages/router/tsconfig.json | Removed Playwright types reference |
| package.json | Added pnpm overrides and new script |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/router/src/router.ts (1)
1013-1025:⚠️ Potential issue | 🟠 MajorTree-shaking concern: components registered unconditionally for Vapor apps.
The global property injection is correctly guarded by
!app.vapor, butRouterLinkandRouterVieware still registered viaapp.component()on lines 1014-1015 regardless of the app type. As noted in PR discussion, this causes Vapor-only apps to pull in the full virtual DOM runtime.Consider extending the vapor check to also skip component registration:
💡 Suggested approach
install(app: App) { - app.component('RouterLink', RouterLink) - app.component('RouterView', RouterView) + if (!app.vapor) { + app.component('RouterLink', RouterLink) + app.component('RouterView', RouterView) + } // TODO: move this part for composition API only if (!app.vapor) {Alternatively, a separate
createVaporRouterentry point could handle Vapor-specific registration as suggested in the PR comments.
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 68-70: The package entries for "vue", "@vue/runtime-dom", and
"@vue/server-renderer" currently point to pkg.pr.new commit URLs; replace those
URL overrides with proper npm version specifiers (for example "3.6.0-beta.5") or
remove the overrides entirely so package.json uses standard npm versions; update
the three keys ("vue", "@vue/runtime-dom", "@vue/server-renderer") to the chosen
semver strings and run npm install / yarn install to verify dependency
resolution.
In `@packages/router/__tests__/mount.ts`:
- Around line 52-84: The test helper createVaporMount mounts a Vapor app but
never calls app.unmount(), risking leaks; modify createVaporMount so the created
app instance is stored in an outer-scope variable and then call app.unmount() in
the afterEach cleanup before removing the DOM element. Specifically, keep the
existing const app = createVaporApp(...) inside the returned mount function but
also assign it to an outer let mountedApp variable (or return an unmount
function), and ensure afterEach calls mountedApp?.unmount() (and clears
mountedApp) so Vapor lifecycle hooks run and state is not retained between
tests.
In `@packages/router/package.json`:
- Line 153: Remove the "vue" entry from the dependencies block in package.json
(the dependency key "vue": "https://pkg.pr.new/vue@d3fca3b") so the package does
not ship a hard-coded PR URL; leave/ensure "vue" only exists in peerDependencies
and devDependencies (the existing "peerDependencies" and "devDependencies"
entries) to keep Vue as a peer and development-only dependency.
🧹 Nitpick comments (6)
packages/router/src/router.ts (1)
1018-1019: Track type definition forapp.vapor.The
@ts-expect-errorsuppression is reasonable while Vapor is experimental. Once Vue 3.6 stabilizes the Vapor API, ensure theAppinterface is properly augmented to include thevaporproperty, or import the correct types from Vue.packages/router/__tests__/VaporRouterView.spec.ts (1)
300-300: Consider renaming describe block to match component name.The test suite is named
'RouterView'but testsVaporRouterView. For clarity and test output readability, consider renaming to'VaporRouterView'.-describe('RouterView', () => { +describe('VaporRouterView', () => {packages/router/__tests__/VaporRouterLink.spec.ts (4)
15-34: Several Vue imports appear unused in this test file.Many Vapor primitives imported from
vue(e.g.,PropType,VaporDirective,setInsertionState,txt) are used in specific inline component definitions within tests. However, consider verifying all imports are necessary. For instance,createComponentWithFallbackappears on line 953 but some others may be unnecessary.
54-59: Inconsistent aliasOf initialization pattern.Lines 54-58 reassign records with
aliasOfproperties using object spread, while line 59 mutatesrecords.childEmptyAliasdirectly. This inconsistency could cause confusion.♻️ Suggested fix for consistency
records.homeAlias = { aliasOf: records.home } as RouteRecordNormalized records.parentAlias = { aliasOf: records.parent, } as RouteRecordNormalized records.childAlias = { aliasOf: records.child } as RouteRecordNormalized -records.childEmptyAlias.aliasOf = records.childEmpty +records.childEmptyAlias = { aliasOf: records.childEmpty } as RouteRecordNormalized
406-443: Inconsistent prop binding syntax across tests.Some tests use getter functions
{ to: () => locations.basic.string }(line 409), while others pass values directly{ to: locations.basic.string }(line 441). While both patterns may work, this inconsistency could mask issues with reactive vs static prop handling.Consider documenting which pattern is intended for which scenario, or standardizing on one approach for clarity.
1012-1013: Consider adding a brief comment explaining the$evtclickpattern.The
@ts-ignoreon line 1012 suppresses a type error for the Vapor-specific event binding pattern. A brief comment explaining this is Vapor's internal event delegation mechanism would help future maintainers.📝 Suggested documentation
- // `@ts-ignore` - n6.$evtclick = e => slotProps0.navigate(e) + // Vapor's event delegation: $evt{eventname} binds click handler + // `@ts-ignore` - internal Vapor API + n6.$evtclick = e => slotProps0.navigate(e)
|
If I include this in a vue-jsx-vapor project and use only |
Description
This PR provide two vapor components:
VaporRouterViewandVaporRouterLink.I'm not sure whether we should provide a
vue-router/vaporfor users to make migration easier.The access globalProperties warning should be remove.
Playground
https://repl.zmjs.dev/vuejs/vue-router
TODO
Summary by CodeRabbit
New Features
Tests
Chores
Updates