fix(runtime-utils): ensure RouterLink stub works with pages: false#1687
fix(runtime-utils): ensure RouterLink stub works with pages: false#1687yamachi4416 wants to merge 1 commit intonuxt:mainfrom
RouterLink stub works with pages: false#1687Conversation
commit: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR introduces a new app4 configuration to the Vitest workspace with separate test scenarios for pages-on and pages-off modes. The primary logic change implements a dynamic fallback mechanism in RouterLink to handle cases where useLink is unavailable in test environments, addressing a regression in RouterLink stub behavior. Additional test suites for NuxtLink and Breadcrumb/Header components are added to app2 and app4. Configuration files (tsconfig, Vitest configs) and package.json scripts are updated to support the new app4 setup and per-app dev:prepare tasks. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The RouterLink.ts change introduces conditional logic with a helper function and fallback rendering path that requires careful analysis (~15 minutes), while the remaining additions—test files, configuration files, and npm scripts—follow consistent patterns with relatively straightforward implementations (~10 minutes). 🚥 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)
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.
🧹 Nitpick comments (1)
src/runtime-utils/components/RouterLink.ts (1)
29-48: 💤 Low valueHoist
useRouter()out of the render function.
useRouter()(and the underlyinguseNuxtApp()) are composables intended to run duringsetup, where the current instance and injection context are guaranteed. Invoking them on every render inside the returned function is both wasteful (re-resolves the Nuxt app/router each tick) and inconsistent with theuseLinkbranch below, which captureslinkonce in setup. The router instance is stable for the component's lifetime, so resolve it once.♻️ Proposed refactor
if (!useLink) { const navigate = () => {} + const router = useRouter() return () => { - const route = useRouter().resolve(props.to) + const route = router.resolve(props.to) return props.custom ? slots.default?.({ href: route.href, navigate, route }) : h( 'a', { href: route.href, onClick: (e: MouseEvent) => { e.preventDefault() return navigate() }, }, slots, ) } }🤖 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 `@src/runtime-utils/components/RouterLink.ts` around lines 29 - 48, Hoist the call to useRouter() out of the inner render so the router instance is captured once during setup (similar to the useLink branch): call const router = useRouter() in the outer scope, then use router.resolve(props.to) inside the returned render function (or compute a reactive/readonly route if needed) instead of calling useRouter() on every render; update references to route.href, navigate, props.to and keep the empty navigate function and custom/anchor render logic unchanged.
🤖 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 `@src/runtime-utils/components/RouterLink.ts`:
- Around line 29-48: Hoist the call to useRouter() out of the inner render so
the router instance is captured once during setup (similar to the useLink
branch): call const router = useRouter() in the outer scope, then use
router.resolve(props.to) inside the returned render function (or compute a
reactive/readonly route if needed) instead of calling useRouter() on every
render; update references to route.href, navigate, props.to and keep the empty
navigate function and custom/anchor render logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6cf3bad4-a06c-4bbc-969b-f1a3ef19791c
📒 Files selected for processing (6)
examples/app-vitest-workspace/app2/nuxt.config.tsexamples/app-vitest-workspace/app2/test/nuxt/components.spec.tsexamples/nuxt-ui/components/SomeBreadcrumb.vueexamples/nuxt-ui/pages/index.vueexamples/nuxt-ui/tests/mount.spec.tssrc/runtime-utils/components/RouterLink.ts
a6420cd to
b203962
Compare
|
I’m going to update the branch and add a few more tests. |
b203962 to
a2ba551
Compare
|
Thank you for the review. |
🔗 Linked issue
resolves #1685
📚 Description
fallback
RouterLinkstub to previous behavior when pages is false