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

Is passing all attributes instead of the delta to validate really a good option? #914

Closed
thedersen opened this issue Jan 31, 2012 · 9 comments
Labels

Comments

@thedersen
Copy link

According to this commit the validate method now receives the computed new state of the attrs, not just the delta.

What this means is that there are no longer any way of knowing which attributes that has changed while performing validation, if I haven't missed something that is.

This is a problem when validating form input while the user is filling out the form. You usually do not want to validate form elements that has not been changed yet, but notify the user about errors on elements that has changed.

Maybe @jashkenas can comment on this?

@jashkenas
Copy link
Owner

If you'd like to check the current state of the model -- it's available in this.

validate: function(attrs) {
  if (!this.get('name')) {
    if (attrs.name.length > 10) return "Name is too long."
  }
},

... in any case, model.validate isn't really best suited for validating HTML forms -- it's best suited for validating the state of your model. If you'd like to validate a form, I'd think that simple function would be easier.

@thedersen
Copy link
Author

The thing is, that using it with form validation works pretty good (using Backbone.Modelbinding and Backbone.Validation plugins), but this change pretty much makes it impossible.

I would definitely not like having to specify validation twice.

@thedersen
Copy link
Author

Btw, since validate effectively prevents invalid values being set on the model, I don't see the need to validate all attributes every time. Attributes not being changed will always be valid.

@threejeez
Copy link

I agree that this is a bit problematic. I understand that the validate method is intended to validate the model, not a form, but there are reasons other than form validation to validate model attributes one at a time. For example, if I have a number of set operations in a row on the same mode, if the first set is invalid I don't want to continue with the following sets; there'd be no point.

Anyway, the workaround is to pass {silent: true} to each of the sets and let the save operation call the validate function.

@jashkenas
Copy link
Owner

Actually yes -- that's exactly how the silent:true operation is supposed to work. Batch up your changes, if desired, and run them all at once later.

@jasonmcaffee
Copy link

Yeah, I've spent a while trying to figure this one out. @thedersen - i'm using your validation library :D
https://github.com/documentcloud/backbone/blob/master/backbone.js#L539

_validate: function(attrs, options) {
      if (options.silent || !this.validate) return true;
      attrs = _.extend({}, this.attributes, attrs);
      var error = this.validate(attrs, options);
  ...

The attrs passed into the _validate function represents the attrs that were changed.
On line 539, the passed in attrs is for some reason merged with this.attributes. (a comment as to why this is occurring would be nice)
this.validate is then called, and ALL attributes are are passed, which ultimately leads to the dom getting modified once for every attributes (in thedersen's validation lib)

I just wrote an override for the Backone._validate function, which essentially justs comments out the attribute merge, and that does the trick, but I'm worried there will be side effects...

Passing in the {silent:true} will just cause the validation to not occur. We need the validation to occur!

The goal is to perform some client side validation, so we don't have to make a round trip to the server.
We want to validate a single input when the user leaves or changes a field, so we can provide immediate feedback indicating that they entered data in a bad format, etc. (eg You have a dollar field, and the user enters 'tacos'. You don't want to make a round trip to the server, when you can instead have some pre-business logic validation occur immediately.

Ultimately we just need an easy way of figuring out which attributes are invalid, and the commit that @thedersen pointed out no longer gives us the ability to do so.

@jacobk
Copy link

jacobk commented Dec 20, 2012

@jashkenas What's your new take on this after 0.9.9 (see your comment above regarding silent:true)?

I've only managed to skim through the delta for 0.9.9 so far, but it looks like there's still validation of all attributes for every Model.set - now without any possibility of skipping the validation (e.g. to incrementally set model attrs before saving)

From the change log
Validation now occurs even during "silent" changes. This change means that the isValid method has been removed.

@tgriesser
Copy link
Collaborator

@jacobk this has actually been a topic of discussion recently - it looks like validation is actually going to be different than both 0.9.2 and 0.9.9 - you can find more details about it in #1972, see if this works for the cases you'll be using validate.

@jacobk
Copy link

jacobk commented Dec 20, 2012

@tgriesser Awesome, I was surprised when I didn't find a more recent issue in the topic.

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

No branches or pull requests

6 participants