Skip to content

Commit

Permalink
initialize empty array of models if there weren't any
Browse files Browse the repository at this point in the history
  • Loading branch information
@dkullmann
dkullmann committed Jun 29, 2012
1 parent 341756f commit f13c2d1
Showing 1 changed file with 7 additions and 1 deletion.
8 changes: 7 additions & 1 deletion backbone.pagedcollection.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,13 @@

_.each(methods, function(method) {
PagedCollection.prototype[method] = function() {
var models = this.pages[ this.page ].collection.models;
var models;
if (typeof this.pages[this.page] !== 'undefined') {
models = this.pages[ this.page ].collection.models;
} else {
models = [];
}

return _[method].apply(_, [models].concat(_.toArray(arguments)));
};
});
Expand Down

6 comments on commit f13c2d1

@GeReV
Copy link

@GeReV GeReV commented on f13c2d1 Jun 29, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to say this seems like a bit of an overkill.
Why perform this check on every call?

Also, I may have addressed this issue lately in my original repository, but can't remember exactly where...

@dkullmann
Copy link
Owner Author

@dkullmann dkullmann commented on f13c2d1 Jun 29, 2012 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GeReV
Copy link

@GeReV GeReV commented on f13c2d1 Jun 29, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I'm just curious, I follow your fork to get ideas.

@dkullmann
Copy link
Owner Author

@dkullmann dkullmann commented on f13c2d1 Jun 30, 2012 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GeReV
Copy link

@GeReV GeReV commented on f13c2d1 Jun 30, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite sure I understand the meaning of the drop in replacement.

But when considering caching only models, you'll have to find a way to track their position in the overall collection, since you could have missing pages.
I originally went with caching collections because it was the simplest solution I could come up with for that specific problem. Each collection is tied to its page number.
Doing that in the model-level seems to be a bit more difficult when you have to complete "holes" in the collection.

As for parameters and URL, there should be something like that in the current version of my fork, if I remember correctly.
Should be something like data and url that you can pass into fetch as options.

There's something I'm trying to solve that you may be able to help with - let's say I have 2 views showing the same collection, but I want one to show 10 items and one to show, say, 8 items.
How would you change the collection so the view can dictate the per-page count?

I have it open as an issue here.

@dkullmann
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"I'm not quite sure I understand the meaning of the drop in replacement."

I mean that you would be able to switch your Collection from extending Backbone.Collection to extending PagedCollection and you wouldn't have to change any other code (in my case I had to change a lot in the view).

"But when considering caching only models, you'll have to find a way to track their position in the overall collection, since you could have missing pages.
I originally went with caching collections because it was the simplest solution I could come up with for that specific problem. Each collection is tied to its page number.
Doing that in the model-level seems to be a bit more difficult when you have to complete "holes" in the collection."

I totally agree with you. I think that first-instinct for anyone here would be to cache the collections. You could still cache the collections by simply storing them in an instance variable on the PagedCollection, and, simply swapping the models from them in / out.

"As for parameters and URL, there should be something like that in the current version of my fork, if I remember correctly.
Should be something like data and url that you can pass into fetch as options."

Okay - awesome - I'll check it out. I added some of this functionality but I made it specific to my project because I just needed to get it up and running. It's pretty clear to me you are a better JS developer to me so I'm sure your fork is better =)

"There's something I'm trying to solve that you may be able to help with - let's say I have 2 views showing the same collection, but I want one to show 10 items and one to show, say, 8 items."

Okay - I'll check out your github issue.

Please sign in to comment.