-
-
Notifications
You must be signed in to change notification settings - Fork 5.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
Adding validateAll feature to Backbone.Model during set or save #1595
Conversation
Hi @gfranko, thanks for the patch! Would you mind explaining why the |
The Let's look at a typical registration form use case that validates form fields using the blur event and validates each field regardless of whether other model attributes (aka other form data) are valid or not. Example desired use case: A user focuses and blurs first name, last name, and email HTML input boxes without entering any data. A "this field is required" message is presented next to each form field. Here is a sample validate method that would be written using the current Backbone validate method:
This method would trigger a first name error each time any of the fields were blurred. This means that only an error message next to the first name field would be presented. Here is a sample validate method that can be accomplished using my change.
This allows my logic inside of my validate method to determine which form fields are currently being set/validated, and does not care about the other model properties that are not trying to be set. |
That's true, but what if you wrote your validate function like this: validate: function(attrs) {
var errors = {};
if (!attrs.firstname) errors.firstname = 'first name is empty';
if (!attrs.lastname) errors.lastname = 'last name is empty';
if (!attrs.email) errors.email = 'email is empty';
if (!_.isEmpty(errors)) return errors;
} You can check the validation for each attribute individually and set the message for the correct blurred field. |
Sure you could validate all fields and return all of the errors, but that forces you to validate all of your form fields every time. Consider if you have ajax calls (w/promises) and other advanced validation methods on some of your form fields. That would mean every time you validate, you could potentially be making an ajax call. |
Well, Backbone expects that |
It really should be an option. Several people have questioned the current implementation. There are use cases where you don't want everything validated, and only want what was changed to be validated. Flexibility is key to a good api. "This method would trigger a first name error each time any of the fields were blurred." I got it to work by overriding the _validate function, but that doesn't seem like a good long term strategy. |
Let's forget about ajax for the moment. If you have multiple client-side validation methods with the above use case, you do not want to have to call each validation method on every attribute every time. This can absolutely be seen as a performance issue. Considering this pull request only adds a few bytes to Backbone and provides an extremely flexible opt-in option, I really believe it should be reopened. |
Hang on a sec -- can't you use Perhaps that should be mentioned more clearly in the docs. |
I thought |
Ah, good point. |
However, you can still do what is described above. Here's a quick example of how you might accomplish this with the technique I described. There is no need for changes to @gfranko Running each attribute through client-side validation should not be performance intensive enough to cause issues. If it is, please post your validation code and we can reconsider this. |
"You want to make the best product possible, and slow products are never the best" --Unknown, taken from zOompf webperf summit Aug 2012. |
I guess my real question is what is the downside to having this option? The upside for me is flexibility and performance. I also think performance intensive enough is very subjective. I added some more validation methods to your jsbin example here and printed out all of the method calls in the console. If you still aren't convinced, maybe I should create a jsPerf test. |
Thanks for posting those @gfranko! Profiling your validate method with |
I appreciate you taking a hard look at this feature @braddunbar . I guess I will have to make a plugin =( Here is the plugin: Backbone.validateAll And just for kicks, here is a jsPerf test: http://jsperf.com/backbone-validateall |
@braddunbar I think the problem with your solution is that model.set won't actually take, even if the particular field is valid. The backbone api is powerful and I want to take full advantage of it. I don't understand the push back on this. The api used to behave this way, but was changed 7 months ago without any option for backwards compatibility. Several people have opened issues on this, and the change is about 3 lines of code. |
I second this pull request -- this should be included. |
👍 This should be merged. |
This commit creates a new
validateAll
option that can be passed when saving or setting one or multiple model properties. A sample use case for this option is form validation.Often during form validation, a developer will not want all model properties to be validated at the same time. Instead, a developer might want to only validate model properties that are currently being set.
The default Model set/save behavior is exactly the same with this addition, and is purely opt-in. Thanks!