refactor(components): [input] use type-based definitions#23366
Conversation
|
|
📝 WalkthroughWalkthroughInput component API and implementation updated: model-modifiers now use presence-based Changes
Sequence Diagram(s)(No sequence diagram generated.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)packages/components/input/src/input.ts (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). (9)
🔇 Additional comments (2)
✏️ Tip: You can disable this entire section by setting 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 |
commit: |
|
Size Change: +1.06 kB (+0.06%) Total Size: 1.65 MB
ℹ️ View Unchanged
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/components/input/src/input.vue (1)
426-438: Potential type mismatch:valuereassigned fromstringtonumber.On line 432,
looseToNumber(value)returns anumber, butvalueis typed asstring. The reassignmentvalue = looseToNumber(value)creates a type inconsistency that TypeScript may not catch due to implicitany. This could propagate incorrect types to emit calls on lines 465-466.🔧 Suggested fix
const formatValue = (value: string) => { const { trim, number } = props.modelModifiers if (trim) { value = value.trim() } + let result: string | number = value if (number) { - value = looseToNumber(value) + result = looseToNumber(value) } if (props.formatter && props.parser) { - value = props.parser(value) + result = props.parser(String(result)) } - return value + return result }Alternatively, update the function signature to return
string | numberexplicitly.
🤖 Fix all issues with AI agents
In @packages/components/package.json:
- Around line 21-22: The package "vue-component-type-helpers" is currently
listed under "dependencies" but is type-only and should be moved to
"devDependencies"; remove the "vue-component-type-helpers": "catalog:" entry
from the "dependencies" object and add the same entry under the
"devDependencies" object in package.json (keeping the same version spec), so the
package is not bundled in production builds.
🧹 Nitpick comments (1)
packages/components/input/src/input.ts (1)
38-41: Minor wording issue in deprecation comment.The deprecation message uses past tense. Consider using future tense for clarity.
📝 Suggested wording
/** - * @deprecated Removed after 3.0.0 + * @deprecated Will be removed after 3.0.0 */Apply the same fix to the deprecation comments at lines 236 and 241.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
docs/en-US/component/input.mdpackages/components/input/src/input.tspackages/components/input/src/input.vuepackages/components/input/src/instance.tspackages/components/package.jsonpnpm-workspace.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
packages/components/input/src/input.ts (1)
packages/constants/size.ts (1)
ComponentSize(3-3)
⏰ 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). (10)
- GitHub Check: Unit Test (New)
- GitHub Check: Unit Test (Current)
- GitHub Check: Unit Test (LTS)
- GitHub Check: Build windows-latest LTS
- GitHub Check: Build ubuntu-latest LTS
- GitHub Check: Lint
- GitHub Check: Coverage
- GitHub Check: SSR rendering test
- GitHub Check: size-report
- GitHub Check: build
🔇 Additional comments (10)
packages/components/input/src/instance.ts (1)
1-6: LGTM!The updated
InputInstancetype correctly combinesComponentInstance(for the full Vue instance) withComponentExposed(for thedefineExposesurface). This provides accurate typing for component refs.docs/en-US/component/input.md (1)
124-124: LGTM!The type change from
booleantotruefor model-modifiers accurately reflects Vue's v-model modifier behavior—modifiers are either present (true) or undefined, never explicitlyfalse.packages/components/input/src/input.vue (3)
163-167: LGTM!The generic script setup pattern
generic="M extends InputModelModifiers = InputModelModifiers"correctly parameterizes the component over model modifier types, enabling type-safe modifier handling.
224-237: LGTM!The
withDefaultspattern withInputProps<M>is well-structured. Default values are appropriate, and the generic type flows correctly through props and emits.
308-312: LGTM!The explicit cast
props.resize as CSSProperties['resize']ensures type safety for the resize CSS property.pnpm-workspace.yaml (1)
21-21: Inconsistent version pinning and potentially incorrect version.This entry uses an exact version
3.2.2while all other catalog entries use caret ranges (e.g.,^3.4.1,^2.3.2). Additionally,3.2.2appears to be higher than the reported npm latest version3.0.6. Verify the package version is correct and either adopt caret range consistency with the rest of the catalog or confirm this exact pinning is intentional for stability.packages/components/input/src/input.ts (4)
12-18: LGTM!The new imports are correctly added to support the type-based interface definitions.
20-24: LGTM!Using
trueliteral type instead ofbooleanis semantically correct for presence-based modifier flags that are either set (true) or absent.
261-300: LGTM!The interface is well-structured with proper typing. The documented workaround for the TS2590 autocomplete union type issue is a reasonable trade-off.
302-322: No changes needed. Theupdate:modelValueemit type is correctly defined asstring | number. Although themodelValueprop acceptsnull | undefined, the component itself never emits these values—it only emits strings and numbers from user interactions. The prop acceptingnull | undefinedis intentional, allowing external value updates from the parent, while the emit type correctly reflects only the values the component produces. The types are consistent with the implementation.Likely an incorrect or invalid review comment.
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
|
🧪 Playground Preview: https://element-plus.run/?pr=23366 |
rzzf
force-pushed
the
refactor/input-generics-comp
branch
from
3b95cc6 to
7132014
Compare
rzzf
force-pushed
the
refactor/input-generics-comp
branch
from
7132014 to
d10f49d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/components/input/src/input.ts (2)
377-377: Missing JSDoc description forrowsprop.The
rowsproperty lacks a@descriptionJSDoc comment, unlike all other properties in this interface. Consider adding one for consistency:📝 Suggested documentation
autofocus?: boolean + /** + * @description number of rows for textarea + */ rows?: number
399-402: Minor: Consider adding a clarifying comment for theeveparameter naming.The commit message mentions renaming
evttoeve. While this is consistent throughout the newInputEmitstype, it's an unconventional abbreviation. A brief comment explaining the naming convention could help future contributors, or consider using the more standardevtor fullevent.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/input/src/input.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/components/input/src/input.ts (1)
packages/constants/size.ts (1)
ComponentSize(3-3)
⏰ 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). (8)
- GitHub Check: size-report
- GitHub Check: Unit Test (Current)
- GitHub Check: Unit Test (New)
- GitHub Check: Unit Test (LTS)
- GitHub Check: Coverage
- GitHub Check: Lint
- GitHub Check: SSR rendering test
- GitHub Check: Build ubuntu-latest LTS
🔇 Additional comments (6)
packages/components/input/src/input.ts (6)
12-18: LGTM!The new imports (
Componentfrom Vue andComponentSizefrom@element-plus/constants) are correctly added to support the new type-basedInputPropsinterface definitions.
20-24: Good refinement of modifier types.Using literal
trueinstead ofbooleanis semantically correct for Vue's v-model modifiers — when a modifier is present, it's alwaystrue; when absent, the property isundefined. This aligns with Vue's actual behavior.Note that this is technically a breaking type change for any consumers who previously typed these as
boolean.
38-41: LGTM!The deprecation notice follows standard JSDoc format and clearly communicates the timeline for removal. This maintains backward compatibility while signaling the transition to the new type-based API.
235-238: LGTM!Consistent deprecation marking with
inputProps.
240-243: LGTM!Consistent deprecation marking with the other runtime definitions.
392-412: Verify the conditional typing logic for theinputevent.The condition
M extends { number: true; lazy?: undefined }for theinputevent seems potentially incorrect. The.lazymodifier affects whenupdate:modelValuefires (onchangeinstead ofinput), but theinputevent itself still fires and receives the value.If the intent is that
.numbermodifier converts the value to a number, then theinputevent should also receivestring | numberwhen.numberis present, regardless of.lazy:- input: [ - value: M extends { number: true; lazy?: undefined } - ? string | number - : string, - ] + input: [ + value: M extends { number: true } ? string | number : string, + ]Please verify the intended behavior with Vue's v-model modifier semantics. If the current logic is intentional (e.g., some internal implementation detail), consider adding a comment explaining why
lazyaffects theinputevent's type.
rzzf
force-pushed
the
refactor/input-generics-comp
branch
from
3a8b691 to
4227fb6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/input/src/input.ts (1)
395-415: Consider the parameter naming convention.The commit message indicates renaming
evttoevewas intentional, buteveis an unconventional abbreviation that may cause confusion (it's also a common proper noun). Consider using standard conventions likeevt,event, ore.Also,
update:modelValue(line 396) always allowsstring | number, whileinputandchangeuse conditional types based on modifiers. For full consistency, consider whetherupdate:modelValueshould also be conditional:'update:modelValue': [value: M extends { number: true } ? string | number : string]However, keeping it as
string | numbermay be intentional for flexibility in edge cases.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/input/src/input.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/components/input/src/input.ts (1)
packages/constants/size.ts (1)
ComponentSize(3-3)
⏰ 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). (10)
- GitHub Check: Coverage
- GitHub Check: Build ubuntu-latest LTS
- GitHub Check: size-report
- GitHub Check: Lint
- GitHub Check: Build windows-latest LTS
- GitHub Check: Unit Test (New)
- GitHub Check: SSR rendering test
- GitHub Check: Unit Test (LTS)
- GitHub Check: Unit Test (Current)
- GitHub Check: build
🔇 Additional comments (6)
packages/components/input/src/input.ts (6)
12-18: LGTM!The added imports (
ComponentandComponentSize) are correctly used in the new type-basedInputPropsinterface for icon props and size prop respectively.
20-24: Good refinement with literal types.Using
trueinstead ofbooleanaccurately represents Vue's v-model modifiers - they're either present (true) or absent (undefined), never explicitlyfalse.
38-41: Clear deprecation notice.The deprecation comment provides a clear timeline for removal. Consider adding migration guidance to the component documentation pointing users to the new
InputProps<M>interface.
240-259: Deprecation notice is consistent.The runtime emits are correctly marked for removal. The transition to the new type-based
InputEmits<M>is well-structured.
261-393: Well-structured type-based interface.The generic
InputProps<M>interface provides clean type-safe definitions. Good documentation of the TS2590 workaround on line 306-308 for theautocompleteproperty.
397-401: Verify the conditional type logic for input event.The condition
M extends { number: true; lazy?: undefined }is non-trivial. Ensure this correctly captures the intended behavior:
- With
.numbermodifier only: emitstring | number- With
.number.lazymodifiers: emitstring(since lazy defers to change event)- Without
.number: emitstringConsider adding a TypeScript test or comment explaining this conditional type for future maintainers:
// When number modifier is active but lazy is not, input events emit parsed numbers. // When lazy is also active, raw string is emitted since number parsing is deferred to change.
Dsaquel
left a comment
Dsaquel
left a comment
There was a problem hiding this comment.
The result looks good, just added some comments.
Co-authored-by: Noblet Ouways <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/components/input/src/input.vue`:
- Around line 224-236: The code uses markRaw for clearIcon in the props default
but markRaw is not imported, causing a build failure; add markRaw to the Vue
imports (the same import statement that provides defineProps/withDefaults or
other Vue helpers) so markRaw is available when setting clearIcon:
markRaw(IconCircleClose) in the props definition.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/input/src/input.vue
🧰 Additional context used
🪛 GitHub Actions: 📦 Package Size Report
packages/components/input/src/input.vue
[error] 230-230: TS2304: Cannot find name 'markRaw'.
🪛 GitHub Actions: Publish PR Commit Pkg
packages/components/input/src/input.vue
[error] 230-230: Cannot find name 'markRaw'. TS2304. This typically indicates missing Vue 3 types or incorrect composition API imports.
🪛 GitHub Check: build
packages/components/input/src/input.vue
[failure] 230-230:
Cannot find name 'markRaw'.
🪛 GitHub Check: Build ubuntu-latest LTS
packages/components/input/src/input.vue
[failure] 230-230:
Cannot find name 'markRaw'.
🪛 GitHub Check: size-report
packages/components/input/src/input.vue
[failure] 230-230:
Cannot find name 'markRaw'.
⏰ 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). (7)
- GitHub Check: Lint
- GitHub Check: SSR rendering test
- GitHub Check: Build windows-latest LTS
- GitHub Check: Coverage
- GitHub Check: Unit Test (LTS)
- GitHub Check: Unit Test (Current)
- GitHub Check: Unit Test (New)
🔇 Additional comments (6)
packages/components/input/src/input.vue (6)
163-167: LGTM!The generic script setup syntax properly enables type-based definitions for the component with the
InputModelModifiersconstraint.
182-186: LGTM!Icon imports are correctly aliased and used for the clear icon default and password visibility toggle.
214-215: LGTM!Type imports properly support the new generic-based prop and emit definitions.
237-237: LGTM!The emit definition correctly applies the generic type parameter for type-safe event emissions.
308-312: LGTM!The explicit cast to
CSSProperties['resize']ensures proper typing for the CSS resize property.
426-438: Type consistency is properly handled by InputEmits generic.The
formatValuefunction parameter is typed asstring, butlooseToNumber(value)returnsany(which may be a number), and this reassignment is intentional when thenumbermodifier is active. TheInputEmits<M>type correctly uses conditional typing to allowstring | numbervalues for theinputandchangeevents only whenM extends { number: true }, ensuring type safety at the emit call sites.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
According to CR review #23366 (comment), |
You’re right. During testing, it didn’t take effect due to pnpm caching, so I added this dependency for them. After retesting, everything works well now. |
|
@rzzf Thanks for your contribution! ❤️ |
3 participants
Try refactoring runtime declarations into type-based declarations.
rel #23172
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.