-
-
Notifications
You must be signed in to change notification settings - Fork 8.9k
fix(v-model): number modifier should update input value #13959
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds a castValue utility and uses it to normalize v-model input and change handlers (supports trim and number modifiers). Tests updated to dispatch a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant I as Input (v-model.number)
participant D as vModel Directive
participant M as Model
participant V as DOM
rect rgb(250,250,255)
note over U,I: Typing (input event)
U->>I: type "+01.2"
I-->>D: input event
D->>D: castValue(el.value, trim?, number?)
D->>M: update modelValue (1.2)
D->>V: may update displayed value
end
rect rgb(245,255,245)
note over U,I: Commit (change/blur)
U->>I: blur / dispatch change
I-->>D: change event
D->>D: castValue(el.value, trim?, number?)
D->>M: assign normalized value
D->>V: set input.value to normalized string "1.2"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
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)
packages/runtime-dom/src/directives/vModel.ts (1)
48-52
: Consider adding an explicit return type for clarity.The function implicitly returns
any
sincelooseToNumber
can return either anumber
or the original value. While this works correctly, adding an explicit return type would improve type safety and code clarity.Consider this refactor:
-function castValue(value: string, trim?: boolean, number?: boolean | null) { +function castValue(value: string, trim?: boolean, number?: boolean | null): string | number { if (trim) value = value.trim() - if (number) value = looseToNumber(value) - return value + if (number) return looseToNumber(value) + return value }This makes it clear that the function can return either a
string
ornumber
, and avoids reassigning thevalue
parameter to a different type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/runtime-dom/src/directives/vModel.ts
(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/runtime-dom/src/directives/vModel.ts (2)
packages/shared/src/general.ts (1)
looseToNumber
(170-173)packages/runtime-dom/src/modules/events.ts (1)
addEventListener
(16-23)
⏰ 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). (4)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
- GitHub Check: test / unit-test-windows
🔇 Additional comments (2)
packages/runtime-dom/src/directives/vModel.ts (2)
66-66
: LGTM!Good refactoring to use the
castValue
utility. This maintains the same behavior as the previous inline logic while improving code clarity and maintainability.
68-72
: Approve runtime-dom vModel change; add or verify testThe change correctly syncs the input value for
v-model.number
on change events, but there’s no existing unit test covering this scenario—please add one for the reproduction case in issue #13958 or verify it manually.
/ecosystem-ci run |
📝 Ran ecosystem CI: Open
|
Do I need to make any further changes to move this PR forward? The errors from VueUse and Vant don’t seem to be related to this PR. |
fix #13958
Summary by CodeRabbit
Bug Fixes
Tests