-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
feat(props): provide props to validation #3258
Conversation
@@ -275,6 +275,28 @@ describe('component props', () => { | |||
expect(root.innerHTML).toBe('<div id="b">2</div>') | |||
}) | |||
|
|||
test('validator arguments', async () => { |
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.
This file looks like the right place but can't see any tests relating to prop validation, nor elsewhere apart from type tests.
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.
Also can't find if there's a type test for the arguments of the validator
?
@@ -466,7 +466,7 @@ function validateProps(props: Data, instance: ComponentInternalInstance) { | |||
for (const key in options) { | |||
let opt = options[key] | |||
if (opt == null) continue | |||
validateProp(key, rawValues[key], opt, !hasOwn(rawValues, key)) | |||
validateProp(key, rawValues[key], opt, rawValues, !hasOwn(rawValues, key)) |
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.
I think this should be cloned deep. Is there an existing utility for that? I could not find one.
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.
Also should the prop value we're originally testing for be filtered out of this rawValues
? (just so there's no confusion why it's available twice)
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.
should mark the rawValues
as shallowReadonly
?
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.
That's perfect. The following still stands:
Also should the prop value we're originally testing for be filtered out of this rawValues? (just so there's no confusion why it's available twice)
Marked it as ready for review, so I can get some feedback |
@HcySunYang can this be re-reviewed please? |
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.
Nice!
Since this is a new feature and it didn't go through an RFC, I think it's worth getting more feedback from the team before merging
Can this PR have couple more reviews on it? |
Any review progression on this PR? I think this is a necessary feature for validator. type input_type_t = 'number' | 'text' | 'password'
const ALL_INPUT_TYPE: input_type_t[] = ['number', 'text', 'password']
// ...
props: {
type: {
default: 'text',
type: String as PropType<input_type_t>,
validator: (x: input_type_t) => ALL_INPUT_TYPE.includes(x)
},
modelValue: { // I want to validate this prop according to the value of `props.type`
required: true,
type: ????????? // Don't know how to refer to the value and TS type information from another prop (`props.type`)
validator: (x: any) => ????????? // Don't know how to refer to the value and TS type information from another prop (`props.type`)
},
} |
@nandi95 this needs to be rebased now |
@posva |
Just want to know the progression of review on this PR? Waiting for this feature for months, currently still do this in setup() manually. |
Can we have some movement on this PR? |
Added all props to the prop validator as a second argument vuejs#3254
18eb597
to
a7b5f1c
Compare
Happy birthday to this PR 🎉 🎉 🎈 |
I want to know why this PR are always be opend since it was born? No malice, just want to know |
❌ Deploy Preview for vue-sfc-playground failed.
|
❌ Deploy Preview for vue-next-template-explorer failed.
|
❌ Deploy Preview for vuejs-coverage failed.
|
@yyx990803 @HcySunYang @pikax |
Size ReportBundles
Usages
|
Added all props to the prop validator as a second argument
closes #3254