fix(routerlink): allow event handlers under strictTemplates (fix #2484) #2548
Conversation
✅ Deploy Preview for vue-router canceled.
|
WalkthroughTypeScript typings for RouterLink were refactored to be generic, differentiating prop shapes when custom is enabled or not. Anchor HTML attributes (excluding href) and DOM events are now typed for non-custom usage. Corresponding DTS tests were added to validate allowed/forbidden props under both modes. No runtime changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/router/src/RouterLink.ts (2)
1-31: Prefer type-only import for AnchorHTMLAttributes to avoid any runtime import riskMove AnchorHTMLAttributes to a separate type-only import for consistency and to keep bundlers from attempting to load it at runtime.
-import { +import { defineComponent, h, PropType, inject, computed, reactive, unref, VNode, UnwrapRef, VNodeProps, AllowedComponentProps, ComponentCustomProps, getCurrentInstance, watchEffect, // this is a workaround for https://github.com/microsoft/rushstack/issues/1050 // this file is meant to be prepended to the generated dist/src/RouterLink.d.ts // @ts-ignore ComputedRef, // @ts-ignore DefineComponent, // @ts-ignore RendererElement, // @ts-ignore RendererNode, // @ts-ignore ComponentOptionsMixin, MaybeRef, - AnchorHTMLAttributes, } from 'vue' +import type { AnchorHTMLAttributes } from 'vue'
369-379: Typing strategy achieves goal; verify behavior with dynamic booleancustombindingsThe conditional props cleanly allow anchor events/attrs when
custom !== trueand exclude them whencustom === true, andhrefis properly omitted. One edge case:<RouterLink :custom="someBool" ...>(or TSXcustom={someBool}wheresomeBool: boolean) may not be assignable to either branch of the union (truevsfalse|undefined), causing a TS error. If that’s acceptable, consider documenting it; otherwise, we may need a broader strategy (tradeoff: allowing dynamic booleans typically re-allows anchor attrs).You can extend the dts test with:
// dynamic boolean: today this likely errors (by design). Decide if we want to support it. const isCustom: boolean = Math.random() > 0.5 // @ts-expect-error: dynamic boolean not supported by narrowed props expectError(<RouterLink to="/" custom={isCustom} onFocus={() => {}} />) // explicit false should allow anchor attrs/handlers expectTypeOf<JSX.Element>(<RouterLink to="/" custom={false} onFocus={() => {}} target="_blank" />) // explicit true should forbid anchor attrs/handlers // @ts-expect-error expectError(<RouterLink to="/" custom={true} onFocus={() => {}} />)packages/router/test-dts/components.test-d.tsx (1)
30-40: Great coverage; add a couple of assertions for explicitcustom={false}and ARIA/data fallthroughTests capture the core behaviors. Consider adding:
- acceptance of
custom={false}with events/attrs- an ARIA/data attribute to ensure broad fallthrough coverage
// allows when explicitly false expectTypeOf<JSX.Element>(<RouterLink to="/" custom={false} onFocus={() => {}} target="_blank" />) // allows ARIA/data attrs expectTypeOf<JSX.Element>(<RouterLink to="/" aria-label="Home" data-testid="rl" />)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/router/src/RouterLink.ts(2 hunks)packages/router/test-dts/components.test-d.tsx(1 hunks)
🔇 Additional comments (1)
packages/router/src/RouterLink.ts (1)
381-383: Generic constructor change LGTMThis preserves inference in JSX while enabling the conditional props. No runtime impact; aligns with the stated objective.
Description
With
vueCompilerOptions.strictTemplates: true, adding any event handler toRouterLink(e.g.@focus) triggers a TS error, even though the handler works at runtime (see #2484).Solution
RouterLinkgeneric overcustomto preserve anchor event/attrs whencustom !== trueand exclude them whencustom === true.Omit<AnchorHTMLAttributes, 'href'>sohrefstays disallowed (we derive it fromto).Tests
Add dts tests to ensure:
customhrefis not allowedcustomistrueAll tests pass:
pnpm -C packages/router build && pnpm -C packages/router build:dtspnpm -C packages/router test:typespnpm -C packages/router test(coverage/unit/e2e)Locally verified with
vue-tsc --noEmitandstrictTemplates: trueon an SFC.Links
Closes #2484
Also happy to adjust if maintainers prefer another approach.
Summary by CodeRabbit