Skip to content
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

Merged
merged 6 commits into from
Dec 5, 2023

Conversation

nandi95
Copy link
Contributor

@nandi95 nandi95 commented Feb 18, 2021

Added all props to the prop validator as a second argument

closes #3254

@@ -275,6 +275,28 @@ describe('component props', () => {
expect(root.innerHTML).toBe('<div id="b">2</div>')
})

test('validator arguments', async () => {
Copy link
Contributor Author

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.

Copy link
Contributor Author

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))
Copy link
Contributor Author

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.

Copy link
Contributor Author

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)

Copy link
Member

@HcySunYang HcySunYang Feb 20, 2021

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?

Copy link
Contributor Author

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)

@nandi95 nandi95 marked this pull request as ready for review February 20, 2021 12:12
@nandi95
Copy link
Contributor Author

nandi95 commented Feb 20, 2021

Marked it as ready for review, so I can get some feedback

@nandi95
Copy link
Contributor Author

nandi95 commented Feb 23, 2021

@HcySunYang can this be re-reviewed please?

Copy link
Member

@posva posva left a 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

@nandi95
Copy link
Contributor Author

nandi95 commented Mar 2, 2021

Can this PR have couple more reviews on it?

@kuanyui
Copy link

kuanyui commented Mar 22, 2021

Any review progression on this PR? I think this is a necessary feature for validator.
I'm making a customed <Input> component like this, but the current API seems no way to refer a prop from another prop:

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`)
    },
}

@HcySunYang HcySunYang added the 🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. label Mar 29, 2021
@HcySunYang
Copy link
Member

@nandi95 this needs to be rebased now

@nandi95
Copy link
Contributor Author

nandi95 commented Jun 6, 2021

@posva
Could you perhaps tag some team members to get some movement on this?

@IS-Kuan
Copy link

IS-Kuan commented Jul 6, 2021

Just want to know the progression of review on this PR? Waiting for this feature for months, currently still do this in setup() manually.

@nandi95
Copy link
Contributor Author

nandi95 commented Aug 6, 2021

Can we have some movement on this PR?

@pikax pikax added the ready to merge The PR is ready to be merged. label Oct 10, 2021
@nandi95
Copy link
Contributor Author

nandi95 commented Feb 18, 2022

Happy birthday to this PR 🎉 🎉 🎈
It's already one year old, can't believe how fast time flies. 🥺🥺
😅

@SnowingFox
Copy link
Contributor

SnowingFox commented Mar 14, 2022

I want to know why this PR are always be opend since it was born? No malice, just want to know

@netlify
Copy link

netlify bot commented Oct 26, 2022

Deploy Preview for vue-sfc-playground failed.

Name Link
🔨 Latest commit a32f7f7
🔍 Latest deploy log https://app.netlify.com/sites/vue-sfc-playground/deploys/6358e9bffab9d70008a6554f

@netlify
Copy link

netlify bot commented Oct 26, 2022

Deploy Preview for vue-next-template-explorer failed.

Name Link
🔨 Latest commit a32f7f7
🔍 Latest deploy log https://app.netlify.com/sites/vue-next-template-explorer/deploys/6358e9bf21070f00096fe8d5

@netlify
Copy link

netlify bot commented Oct 26, 2022

Deploy Preview for vuejs-coverage failed.

Name Link
🔨 Latest commit a32f7f7
🔍 Latest deploy log https://app.netlify.com/sites/vuejs-coverage/deploys/6358e9bf21070f00096fe8d0

@vuejs vuejs deleted a comment from vue-bot Feb 10, 2023
@nandi95
Copy link
Contributor Author

nandi95 commented Mar 4, 2023

@yyx990803 @HcySunYang @pikax
Can we get any movement on this?

@github-actions
Copy link

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 85.9 kB 32.6 kB 29.5 kB
vue.global.prod.js 132 kB 49.3 kB 44.4 kB

Usages

Name Size Gzip Brotli
createApp 47.9 kB 18.8 kB 17.2 kB
createSSRApp 50.6 kB 19.9 kB 18.2 kB
defineCustomElement 50.3 kB 19.6 kB 17.9 kB
overall 61.2 kB 23.7 kB 21.6 kB

@yyx990803 yyx990803 changed the base branch from main to minor December 5, 2023 09:11
@yyx990803 yyx990803 merged commit 8e27692 into vuejs:minor Dec 5, 2023
2 checks passed
@nandi95 nandi95 deleted the feat/prop-arguments branch December 5, 2023 10:06
@Lehoczky Lehoczky mentioned this pull request Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. ready to merge The PR is ready to be merged. version: minor
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Prop cross-validation
10 participants