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

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@gfranko

gfranko commented Aug 28, 2012

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!

@braddunbar

This comment has been minimized.

Show comment
Hide comment
@braddunbar

braddunbar Aug 29, 2012

Collaborator

Hi @gfranko, thanks for the patch! Would you mind explaining why the _.extend call in question prevents you from doing form validation (or why it is made difficult)?

Collaborator

braddunbar commented Aug 29, 2012

Hi @gfranko, thanks for the patch! Would you mind explaining why the _.extend call in question prevents you from doing form validation (or why it is made difficult)?

@gfranko

This comment has been minimized.

Show comment
Hide comment
@gfranko

gfranko Aug 29, 2012

The _.extend call in question passes all of the model attributes to the validate method every time, regardless of the actual model attributes being set.

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:

validate: function(attrs) {

    if(!attrs.firstname) {
         console.log('first name is empty');
         return false;
    }

    if(!attrs.lastname) {
        console.log('last name is empty');
        return false;
    }

    if(!attrs.email) {
        console.log('email is empty');
        return false;
    }

}

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.

validate: function(attrs) {

     if(attrs.firstname != null) {
         if(attrs.firstname.length === 0) {
             console.log('first name is empty');
             return false;
         }
     }

     if(attrs.lastname != null) {
         if(attrs.lastname.length === 0) {
             console.log('last name is empty');
             return false;
         }
     }

     if(attrs.email != null) {
         if(attrs.email.length === 0) {
             console.log('email is empty');
             return false;
         }
     }

}

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.

gfranko commented Aug 29, 2012

The _.extend call in question passes all of the model attributes to the validate method every time, regardless of the actual model attributes being set.

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:

validate: function(attrs) {

    if(!attrs.firstname) {
         console.log('first name is empty');
         return false;
    }

    if(!attrs.lastname) {
        console.log('last name is empty');
        return false;
    }

    if(!attrs.email) {
        console.log('email is empty');
        return false;
    }

}

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.

validate: function(attrs) {

     if(attrs.firstname != null) {
         if(attrs.firstname.length === 0) {
             console.log('first name is empty');
             return false;
         }
     }

     if(attrs.lastname != null) {
         if(attrs.lastname.length === 0) {
             console.log('last name is empty');
             return false;
         }
     }

     if(attrs.email != null) {
         if(attrs.email.length === 0) {
             console.log('email is empty');
             return false;
         }
     }

}

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.

@braddunbar

This comment has been minimized.

Show comment
Hide comment
@braddunbar

braddunbar Aug 29, 2012

Collaborator

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.

Collaborator

braddunbar commented Aug 29, 2012

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.

@gfranko

This comment has been minimized.

Show comment
Hide comment
@gfranko

gfranko Aug 29, 2012

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.

gfranko commented Aug 29, 2012

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.

@braddunbar

This comment has been minimized.

Show comment
Hide comment
@braddunbar

braddunbar Aug 29, 2012

Collaborator

Well, Backbone expects that validate is a synchronous method so if you're making ajax requests you should do it in a separate, custom method. validate is meant to do client-side validation, not server-side validation. For server-side validation, you can return an appropriately failing HTTP status to the save request and update the UI accordingly.

Collaborator

braddunbar commented Aug 29, 2012

Well, Backbone expects that validate is a synchronous method so if you're making ajax requests you should do it in a separate, custom method. validate is meant to do client-side validation, not server-side validation. For server-side validation, you can return an appropriately failing HTTP status to the save request and update the UI accordingly.

@braddunbar braddunbar closed this Aug 29, 2012

@jasonmcaffee

This comment has been minimized.

Show comment
Hide comment
@jasonmcaffee

jasonmcaffee Aug 29, 2012

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."
That's exactly what I was trying to do. Here are some related issues:
thedersen/backbone.validation#54
#914
//#1581

I got it to work by overriding the _validate function, but that doesn't seem like a good long term strategy.

jasonmcaffee commented Aug 29, 2012

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."
That's exactly what I was trying to do. Here are some related issues:
thedersen/backbone.validation#54
#914
//#1581

I got it to work by overriding the _validate function, but that doesn't seem like a good long term strategy.

@gfranko

This comment has been minimized.

Show comment
Hide comment
@gfranko

gfranko Aug 29, 2012

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.

gfranko commented Aug 29, 2012

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.

@jashkenas

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Aug 29, 2012

Owner

Hang on a sec -- can't you use this.changedAttributes() within validate, if you want this behavior?

Perhaps that should be mentioned more clearly in the docs.

Owner

jashkenas commented Aug 29, 2012

Hang on a sec -- can't you use this.changedAttributes() within validate, if you want this behavior?

Perhaps that should be mentioned more clearly in the docs.

@gfranko

This comment has been minimized.

Show comment
Hide comment
@gfranko

gfranko Aug 29, 2012

I thought changedAttributes() is supposed to tell you what model attributes have changed after validation?

gfranko commented Aug 29, 2012

I thought changedAttributes() is supposed to tell you what model attributes have changed after validation?

@jashkenas

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Aug 29, 2012

Owner

Ah, good point.

Owner

jashkenas commented Aug 29, 2012

Ah, good point.

@braddunbar

This comment has been minimized.

Show comment
Hide comment
@braddunbar

braddunbar Aug 29, 2012

Collaborator

@jashkenas No, you can't use model.changed because validate is called before values are updated. Sorry, @gfranko beat me to it. :)

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 _validate.

@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.

Collaborator

braddunbar commented Aug 29, 2012

@jashkenas No, you can't use model.changed because validate is called before values are updated. Sorry, @gfranko beat me to it. :)

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 _validate.

@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.

@netvisao

This comment has been minimized.

Show comment
Hide comment
@netvisao

netvisao Aug 29, 2012

"You want to make the best product possible, and slow products are never the best"

--Unknown, taken from zOompf webperf summit Aug 2012.

netvisao commented Aug 29, 2012

"You want to make the best product possible, and slow products are never the best"

--Unknown, taken from zOompf webperf summit Aug 2012.

@gfranko

This comment has been minimized.

Show comment
Hide comment
@gfranko

gfranko Aug 29, 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.

gfranko commented Aug 29, 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.

@braddunbar

This comment has been minimized.

Show comment
Hide comment
@braddunbar

braddunbar Aug 29, 2012

Collaborator

Thanks for posting those @gfranko!

Profiling your validate method with console.profile and console.profileEnd, it takes 1-2ms to run. That's definitely fast enough that it won't noticeably affect user experience. Further, I don't think it justifies the addition of a new option.

Collaborator

braddunbar commented Aug 29, 2012

Thanks for posting those @gfranko!

Profiling your validate method with console.profile and console.profileEnd, it takes 1-2ms to run. That's definitely fast enough that it won't noticeably affect user experience. Further, I don't think it justifies the addition of a new option.

@gfranko

This comment has been minimized.

Show comment
Hide comment
@gfranko

gfranko Aug 29, 2012

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

gfranko commented Aug 29, 2012

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

@jasonmcaffee

This comment has been minimized.

Show comment
Hide comment
@jasonmcaffee

jasonmcaffee Aug 30, 2012

@braddunbar I think the problem with your solution is that model.set won't actually take, even if the particular field is valid.
This means I can't listen for change events on the model, and respond to user inputs by refreshing other parts of the screen. (eg user selects an item from 1 select box, and another select box gets refreshed)

The backbone api is powerful and I want to take full advantage of it.
The change event won't fire in your example.

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.
Seems like an easy win for the backbone api.

jasonmcaffee commented Aug 30, 2012

@braddunbar I think the problem with your solution is that model.set won't actually take, even if the particular field is valid.
This means I can't listen for change events on the model, and respond to user inputs by refreshing other parts of the screen. (eg user selects an item from 1 select box, and another select box gets refreshed)

The backbone api is powerful and I want to take full advantage of it.
The change event won't fire in your example.

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.
Seems like an easy win for the backbone api.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 6, 2012

I second this pull request -- this should be included.

ghost commented Sep 6, 2012

I second this pull request -- this should be included.

@digitalmaster

This comment has been minimized.

Show comment
Hide comment
@digitalmaster

digitalmaster Aug 10, 2013

👍 This should be merged.

digitalmaster commented Aug 10, 2013

👍 This should be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment