fix(types): Directive Arg type in packages/runtime-core/src/directives #13758
Conversation
WalkthroughRelaxed TypeScript constraints for directive argument generics across runtime-core and tests: replaced Changes
Sequence Diagram(s)(omitted — changes are type-level only and do not modify runtime control flow) Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these settings in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
packages/runtime-core/src/directives.ts (1)
124-131: Widen directive arg types: update DirectiveArguments and generic Arg in apiCreateApp.tsDirectiveArguments still forces the arg to string. ripgrep shows remaining generic constraints (Arg extends string) in apiCreateApp.ts — these must be loosened to allow non-string args.
Files to update:
- packages/runtime-core/src/directives.ts — change the tuple variants that force string to accept any.
- packages/runtime-core/src/apiCreateApp.ts — change both occurrences of
Arg extends string = stringtoArg = any.Suggested diffs:
--- a/packages/runtime-core/src/directives.ts +++ b/packages/runtime-core/src/directives.ts @@ export type DirectiveArguments = Array< | [Directive | undefined] | [Directive | undefined, any] - | [Directive | undefined, any, string] - | [Directive | undefined, any, string | undefined, DirectiveModifiers] + | [Directive | undefined, any, any] + | [Directive | undefined, any, any, DirectiveModifiers] >--- a/packages/runtime-core/src/apiCreateApp.ts +++ b/packages/runtime-core/src/apiCreateApp.ts @@ - Value = any, - Modifiers extends string = string, - Arg extends string = string, + Value = any, + Modifiers extends string = string, + Arg = any, )( name: string,(Apply the same replacement for the second generic block found near lines 59–63.)
🧹 Nitpick comments (1)
packages/runtime-core/src/directives.ts (1)
38-38: Propagate Modifiers and Arg generics into the dir property for stronger typingCarrying the generics here preserves the directive’s full type information on the binding (particularly Arg), improving inference in hooks.
Apply this diff:
- dir: ObjectDirective<any, Value> + dir: ObjectDirective<any, Value, Modifiers, Arg>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/runtime-core/src/directives.ts(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/runtime-core/src/directives.ts (1)
packages/runtime-core/src/index.ts (2)
DirectiveHook(327-327)Directive(325-325)
🔇 Additional comments (6)
packages/runtime-core/src/directives.ts (6)
31-31: Relaxing Arg to any in DirectiveBinding is correct and aligns with real-world directivesThis change directly addresses the linked issue and supports non-string args (e.g., HTMLElement) in bindings.
46-50: DirectiveHook Arg widened to any — good callLoosening Arg here is consistent with DirectiveBinding and unblocks directives that use non-string args.
55-60: SSRDirectiveHook Arg widened to any — consistent with runtime hooksKeeps SSR hooks aligned with runtime bindings and prevents unnecessary constraints.
67-76: ObjectDirective Arg widened to any — consistent and non-breakingThis maintains parity across directive shapes and avoids artificial constraints.
102-103: FunctionDirective Arg widened to any — consistent changeMatches ObjectDirective and DirectiveHook, ensuring uniformity.
109-112: Directive union Arg widened to any — aligns with upstream usageCorrectly reflects that Arg can be non-string across object and function directives.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/runtime-core/src/apiCreateApp.ts (1)
18-19: Sanity check — lingeringArg extends stringfoundrg found one remaining occurrence in a DTS test; please remove/update the constraint to avoid partial inconsistency.
- packages-private/dts-test/directives.test-d.ts:16 —
Arg extends string = string,Suggested change:
- Before:
Arg extends string = string,- After:
Arg = string,
🧹 Nitpick comments (2)
packages/runtime-core/src/directives.ts (2)
33-39: Consider parameterizing DirectiveBinding.dir to preserve Modifiers and Arg generics.Currently
diris typed asObjectDirective<any, Value>, which drops theModifiersandArgparameters you now expose viaDirectiveBinding. This weakens type propagation to hooks (they’ll default tostringandany). If there are no known TS bugs blocking it, tighten it as below to preserve the full generic info:- modifiers: DirectiveModifiers<Modifiers> - dir: ObjectDirective<any, Value> + modifiers: DirectiveModifiers<Modifiers> + dir: ObjectDirective<any, Value, Modifiers, Arg>If this was intentionally wide to avoid prior TS issues (the nearby __mod workaround hints at that), feel free to skip; otherwise this improves type inference end-to-end.
124-131: Allowing non-string DirectiveArguments arg is appropriate; consider adding a d.ts test.The tuple updates to
[Directive | undefined, any, any]and[Directive | undefined, any, any, DirectiveModifiers]correctly reflect the loosenedArg. To guard against regression, consider adding a test in directives.test-d.ts asserting HTMLElement is accepted asargfor both object and function directives.I can add a minimal d.ts test case that uses
withDirectivesandApp.directivewith an HTMLElementarg. Want me to draft it?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/runtime-core/src/apiCreateApp.ts(1 hunks)packages/runtime-core/src/directives.ts(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/runtime-core/src/apiCreateApp.ts (1)
packages/runtime-core/src/directives.ts (1)
Directive(105-112)
🔇 Additional comments (2)
packages/runtime-core/src/directives.ts (1)
31-37: Relaxing Arg to any across directive types is correct and aligns with the reported issue.This matches the goal from #13757 and enables non-string args (e.g., HTMLElement) without impacting runtime behavior. Consistent updates across DirectiveBinding, DirectiveHook, SSRDirectiveHook, ObjectDirective, FunctionDirective, and Directive are good.
Also applies to: 46-53, 57-62, 67-96, 98-104, 105-113
packages/runtime-core/src/apiCreateApp.ts (1)
49-57: App.directive overloads now consistent with relaxed Arg = any.The widened Arg default in both overloads aligns with the updated Directive types and resolves the inconsistency. No runtime impact.
Also applies to: 58-66
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/runtime-core/src/directives.ts (1)
105-113: Sanity check — one lingeringArg extends stringfound; please widen itFound one remaining string-constrained Arg in the d.ts test; runtime types and App.directive overloads look fine. Please update the DTS/test typing.
- packages-private/dts-test/directives.test-d.ts:16 — contains
Arg extends string = string(change toArg = anyor remove theextends stringconstraint)- packages/runtime-core/src/directives.ts: lines ~28, 63, 98, 105 — confirmed DirectiveBinding / ObjectDirective / FunctionDirective / Directive use
Arg = any- packages/runtime-core/src/compat/global.ts:209,212 — App.directive usages found; no narrowing of
Argdetected- packages/runtime-core/tests/directives.spec.ts:24, 159, 213 — tests assert
binding.argruntime value (OK), but update corresponding d.ts if you widen types- packages/runtime-core/src/index.ts — no re-export narrowing of Directive types found
🧹 Nitpick comments (2)
packages/runtime-core/src/directives.ts (2)
63-96: Add dts coverage for a non-string Arg to prevent regressionsConsider adding a directives.test-d.ts case where Arg is a non-string (e.g., HTMLElement) to lock in this behavior.
Example snippet for dts tests:
import type { ObjectDirective } from '../../src' type ArgEl = HTMLElement const vOutside: ObjectDirective<HTMLElement, any, string, ArgEl> = { mounted(el, binding) { const maybeEl = binding.arg // should be HTMLElement | undefined const _assert: ArgEl | undefined = maybeEl }, }Would you like me to draft a full test file following the repo’s dts test conventions?
31-31: Optional: keep default Arg = string (no extends) to preserve DX while allowing non-string overridesIf you want to retain the common-case ergonomics (binding.arg defaults to string in userland) but still support non-string args when explicitly specified, consider removing the extends constraint but keeping the default as string. This avoids an abrupt widening to any in the default experience, yet remains flexible when a directive chooses a different Arg type.
Apply across the touched declarations:
-interface DirectiveBinding< - Value = any, - Modifiers extends string = string, - Arg = any, -> { +interface DirectiveBinding< + Value = any, + Modifiers extends string = string, + Arg = string, +> { ... - dir: ObjectDirective<any, Value, Modifiers, Arg> + dir: ObjectDirective<any, Value, Modifiers, Arg> } -type DirectiveHook< +type DirectiveHook< HostElement = any, Prev = VNode<any, HostElement> | null, Value = any, Modifiers extends string = string, - Arg = any, + Arg = string, > = ( ... ) => void -type SSRDirectiveHook< +type SSRDirectiveHook< Value = any, Modifiers extends string = string, - Arg = any, + Arg = string, > = ( ... ) => Data | undefined -export interface ObjectDirective< +export interface ObjectDirective< HostElement = any, Value = any, Modifiers extends string = string, - Arg = any, + Arg = string, > { ... } -export type FunctionDirective< +export type FunctionDirective< HostElement = any, V = any, Modifiers extends string = string, - Arg = any, + Arg = string, > = DirectiveHook<HostElement, any, V, Modifiers, Arg> -export type Directive< +export type Directive< HostElement = any, Value = any, Modifiers extends string = string, - Arg = any, + Arg = string, > = | ObjectDirective<HostElement, Value, Modifiers, Arg> | FunctionDirective<HostElement, Value, Modifiers, Arg>Note: Keeping DirectiveArguments’ 3rd tuple element as any still allows passing non-string args through withDirectives, while directive authors can opt into a non-string Arg by parameterizing their directive types.
Also applies to: 38-38, 46-46, 57-57, 67-67, 102-102, 109-109
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/runtime-core/src/directives.ts(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/runtime-core/src/directives.ts (3)
packages/runtime-core/src/index.ts (5)
ComponentPublicInstance(299-299)ObjectDirective(328-328)DirectiveHook(327-327)VNode(250-250)Directive(325-325)packages/runtime-core/src/componentPublicInstance.ts (1)
ComponentPublicInstance(292-345)packages/runtime-core/src/vnode.ts (1)
VNode(160-257)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
🔇 Additional comments (7)
packages/runtime-core/src/directives.ts (7)
28-39: Arg generic relaxed and propagated to dir: aligns with the linked issuesChanging Arg to any and wiring it through dir fixes the overly strict constraint reported in #13757 and unblocks non-string args (e.g., HTMLElement in element-plus vClickOutside). No runtime impact.
41-53: DirectiveHook now supports non-string argWidening Arg to any on the hook signature is consistent with the binding change and avoids downstream typing friction.
54-62: SSRDirectiveHook arg widened to anyMatches runtime hook shape and keeps SSR parity with client-side types. Looks good.
63-96: ObjectDirective generics propagate the widened Arg correctly across lifecycle hooksArg = any flows through created/mounted/updated/... and getSSRProps. This fully resolves the constraint and preserves existing behavior.
98-104: FunctionDirective supports non-string argThe single-function directive form is updated consistently. No concerns.
105-113: Directive union widened ArgThe top-level Directive alias now accepts non-string args across both object and function forms. This harmonizes the entire directive surface.
124-131: DirectiveArguments tuple shape widened for argSwitching the 3rd tuple element to any ensures withDirectives can accept non-string args. Note: undefined remains assignable to any, so prior call sites that passed undefined as the third element continue to type-check.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages-private/dts-test/directives.test-d.ts (1)
35-41: Incorrect modifiers expectation for Modifiers='a' caseWhen calling testDirective<number, 'a', 'Arg'>(), the modifiers type should only include 'a', not 'a' | 'b'.
Apply:
- modifiers: Partial<Record<'a' | 'b', boolean>> + modifiers: Partial<Record<'a', boolean>>
🧹 Nitpick comments (1)
packages-private/dts-test/directives.test-d.ts (1)
49-55: Duplicate test block; remove to avoid redundancyThis block duplicates the assertion already covered above (Lines 42-48). Removing reduces noise without losing coverage.
Apply:
- expectType<{ - value: number - oldValue: number | null - arg?: 'Arg' - modifiers: Partial<Record<'a' | 'b', boolean>> - }>(testDirective<number, 'a' | 'b', 'Arg'>())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages-private/dts-test/directives.test-d.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
🔇 Additional comments (3)
packages-private/dts-test/directives.test-d.ts (3)
42-48: LGTM: union modifiers correctly reflected with Partial<Record<...>>This validates union modifier keys and optional presence as intended.
56-62: LGTM: coverage for non-string Arg (HTMLElement) is correctConfirms Arg can be a DOM element and flows into binding.arg as expected.
13-17: Broadened Arg generic to any — verified and approvedRan the requested repo search for "Arg extends string" — no matches. The helper signature in packages-private/dts-test/directives.test-d.ts now uses Arg = any and aligns with Directive usage in packages/runtime-core. LGTM.
czhlin
changed the title
Size ReportBundles
Usages
|
czhlin
force-pushed
the
fix/directive-type
branch
from
5cf5698 to
305b615
Compare
closes #13757, closes element-plus/element-plus/#21729
Summary by CodeRabbit
Refactor
Tests