Move shouldFetch to model and collection instances #90

Merged
merged 2 commits into from Jan 3, 2013

2 participants

@eastridge
Walmart Labs member

Fairly straightforward, I don't see impact on Phoenix as it turns out we aren't using forceFetch.

Two questions:

  1. If this pull request is rejected: jashkenas/backbone#2057 what is the real impact of keeping a try / catch block? The code is far simpler and smaller to implement this way than overriding url.

  2. Should we explicitly disable support for vanilla Backbone.Model and Backbone.Collection instances being passed to bindDataObject, presently the if (dataObject.shouldFetch) { check is kept around for that. If we do explicitly disable support for that we will need to throw an error instead of doing a noop.

@eastridge
Walmart Labs member

Pull request jashkenas/backbone#2057 rejected. I don't see the problem with using a try / catch, @kpdecker, thoughts?

@kpdecker kpdecker commented on the diff Jan 2, 2013
src/collection.js
@@ -19,6 +19,9 @@ Thorax.Collection = Backbone.Collection.extend({
isPopulated: function() {
return this._fetched || this.length > 0 || (!this.length && !_.result(this, 'url'));
},
+ shouldFetch: function(options) {
+ return options.fetch && !!_.result(this, 'url') && !this.isPopulated();
@kpdecker
kpdecker added a note Jan 2, 2013

nit: !! isn't necessary and !!_. feels clunky.

@eastridge
Walmart Labs member
@kpdecker
kpdecker added a note Jan 3, 2013

This will always be a boolean due to the && operators (or you could have something falsy but meh). Besides that, this isn't a "secure system" so who cares if a truthy value is exposed rather than true?

@eastridge
Walmart Labs member

I'd rather leave this as is, but that's not my understanding of how the boolean operators work in JS.

('a' && 'b') === 'b'

Not true. _.result(this, 'url') will either be a string or falsy.

@kpdecker
kpdecker added a note Jan 3, 2013

!this.isPopulated is going to make the result boolean if all truthy and worst case you get a falsy value if result is falsy. What is the issue thruthy and falsy?

@eastridge
Walmart Labs member
@kpdecker
kpdecker added a note Jan 4, 2013

Types are meaningless :) But then again we are spending a lot of time on two chars that are already commited and basically just a stylistic concern. Willing to drop it :)

@eastridge
Walmart Labs member
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kpdecker kpdecker commented on the diff Jan 2, 2013
src/model.js
@@ -17,6 +17,17 @@ Thorax.Model = Backbone.Model.extend({
}
var keys = _.keys(attributes);
return keys.length > 1 || (keys.length === 1 && keys[0] !== this.idAttribute);
+ },
+ shouldFetch: function(options) {
+ // TODO: if https://github.com/documentcloud/backbone/pull/2057 is merged
+ // then add {silent: true} to url call
+ var url;
+ try {
+ url = this.url();
+ } catch(e) {
+ url = false;
+ }
@kpdecker
kpdecker added a note Jan 2, 2013

Can you walk me through this try/catch? What is it doing, why is it there?

@eastridge
Walmart Labs member
@kpdecker
kpdecker added a note Jan 3, 2013

Can you add a comment to this effect in the code?

@eastridge
Walmart Labs member

Adding comment in and merging.

Boo Backbone pull requests! 💩

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

On the Backbone object support I personally don't see a huge benefit to supporting it other than for dealing with other backbone plugins or legacy code that couldn't be ported/monkey patched.

Is that if conditional the only thing that is necessary to support backbone objects rather than thorax objects?

@eastridge
Walmart Labs member
@eastridge eastridge merged commit d08989c into master Jan 3, 2013

1 check was pending

Details default The Travis build is in progress
@eastridge eastridge deleted the shouldFetch branch Jan 3, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment