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: add extendValidator #12738

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

btea
Copy link
Contributor

@btea btea commented Jan 18, 2025

Sometimes the type matches but the range does not match. I hope to give a clearer and more explicit error prompt.

Copy link

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 100 kB 38 kB 34.2 kB
vue.global.prod.js 158 kB 57.8 kB 51.4 kB

Usages

Name Size Gzip Brotli
createApp (CAPI only) 46.4 kB 18.2 kB 16.6 kB
createApp 54.3 kB 21.2 kB 19.3 kB
createSSRApp 58.5 kB 22.9 kB 20.9 kB
defineCustomElement 59.2 kB 22.8 kB 20.7 kB
overall 68.4 kB 26.3 kB 24 kB

Copy link

pkg-pr-new bot commented Jan 18, 2025

Open in Stackblitz

@vue/compiler-core

npm i https://pkg.pr.new/@vue/compiler-core@12738

@vue/compiler-dom

npm i https://pkg.pr.new/@vue/compiler-dom@12738

@vue/compiler-sfc

npm i https://pkg.pr.new/@vue/compiler-sfc@12738

@vue/reactivity

npm i https://pkg.pr.new/@vue/reactivity@12738

@vue/compiler-ssr

npm i https://pkg.pr.new/@vue/compiler-ssr@12738

@vue/runtime-core

npm i https://pkg.pr.new/@vue/runtime-core@12738

@vue/runtime-dom

npm i https://pkg.pr.new/@vue/runtime-dom@12738

@vue/server-renderer

npm i https://pkg.pr.new/@vue/server-renderer@12738

@vue/shared

npm i https://pkg.pr.new/@vue/shared@12738

vue

npm i https://pkg.pr.new/vue@12738

@vue/compat

npm i https://pkg.pr.new/@vue/compat@12738

commit: 942328e

@Justineo
Copy link
Member

Justineo commented Jan 19, 2025

I'm afraid this would be a breaking change as returning a non-empty string used to mean validation success.

I like the idea and would love for Vue to include a built-in solution that allows developers to leverage existing utilities for handling prop validation errors. To be honest, I’ve always thought the custom validator should be named validate. If we can adopt this new name, it could open up new possibilities for refining the API. But I think this should be discussed in our RFC report first.

@btea
Copy link
Contributor Author

btea commented Jan 19, 2025

Thanks for your reply.

I'm afraid this would be a breaking change as returning a non-empty string used to mean validation success.

Yes, this is indeed a problem.

To be honest, I’ve always thought the custom validator should be named validate. If we can adopt this new name, it could open up new possibilities for refining the API.

IMO, renaming existing properties will have a big impact. To avoid afferting users, we should either add a new method to customize the validation rules, or add a new parameter to the existing validator function for special handling.

But I think this should be discussed in our RFC report first.

Would you like me to create an issue in the RFC repository first?

@Justineo
Copy link
Member

I'm not suggesting renaming, but add a new alternative with enhanced behavior.

And I believe a separate RFC thread is necessary.

@btea btea changed the title feat: validator support return string feat: add extendValidator Jan 19, 2025
@btea
Copy link
Contributor Author

btea commented Jan 19, 2025

I'm not suggesting renaming, but add a new alternative with enhanced behavior.

Sorry, I misunderstood what you meant.

And I believe a separate RFC thread is necessary.

link vuejs/rfcs#739

@baiwusanyu-c
Copy link
Member

I prefer to extend the parameters based on validator to control its behavior, and reduce the generation of new APIs while ensuring compatibility. Too many APIs are not necessarily a good thing. 🧐

@btea
Copy link
Contributor Author

btea commented Jan 20, 2025

Yeah, at first I just wanted to simply expand the warning message, so I created this PR in a rough way.

After discussing with @jacekkarczmarczyk, I am still a little undecided about how to implement this requirement. Maybe we need to discuss it together and confirm it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants