requestPager break if fetch() is not the first function called #99

Closed
SBoudrias opened this Issue Oct 29, 2012 · 23 comments

Projects

None yet

7 participants

@SBoudrias
Member

Hi,

Most of the functions used internally by requestPager refer to the paginator_ui value but on the collection itself (e.g. this.currentPage, example).

If we follow the settings described in the doc, one would only set paginator_ui hash then call functions. Although, most of the function will break as at this point the collection haven't been extended with paginator_ui. This only happen in the async function.

Doing so, requestPager will only work if .fetch() is the first method called on it (as this will run async, and as it's the only function not blocking on configuration error).

Right now, if one don't want to use fetch() to init the collection, he must setup manually the paginator_ui directly on the collection, or extend it as in the async function while initialize() is runned.

I'm not sure what could be the better fix for it, but I think it feels clumpsy that paginator_ui is duplicated on the collection itself. I'd probably go for one or the other and remove duplication (only paginator_ui or only attributes set directly on the collection).

Let me know what you think

@skusunam
Member
skusunam commented Dec 5, 2012

@SBoudrias Do you have any use case where you do not want call .fetch()? Trying to understand the problem.

@SBoudrias
Member

Yeah, when you get data printed out in the page layout on load for example. So you'll initialize you collection like so

var collection = new MyCollection( data );

Then you have no need to call fetch() and may want to call requestNextPage() first.

@gilbertwyw

@skusunam any use case i can come up with is when i use the collection in AMD style (require.js)
i tend to instantiate it when return the collection.

@skusunam
Member
skusunam commented Feb 5, 2013

@willsu This issue should be fixed with your PR (bootstrap): #131 right?
cc/ @alexandernst @addyosmani

@willsu
Contributor
willsu commented Feb 6, 2013

@skusunam Yes, that is correct. @SBoudrias, please reference the 'Bootstrapping' section of the README.md: https://github.com/addyosmani/backbone.paginator#bootstrapping. Hopefully this should solve your problem (works with both the client and request paginator).

@SBoudrias
Member

I haven't follow up with the Backbone paginator recently, so I don't know what led you to this choice; but personnally I feel like there's some overhead in the API. If the collection isn't empty, shouldn't it be implied that the paginator been boostrapped?

Anyway, as there's now a solution to the issue, I'll close it.

@SBoudrias SBoudrias closed this Feb 6, 2013
@willsu
Contributor
willsu commented Feb 6, 2013

@SBoudrias You are correct in that the initial request could probably be omitted if the collection is non-empty. However, in order to correctly calculate the perPage, totalPages, etc, the requestPager will need additional information (namely totalRecords). That said, the totalRecords property needs to be set by means on the requestPager. The bootstrap function accepts arbitrary properties like totalRecords and returns an instance of this when the function call returns. That way, initializing the collection becomes a one-liner.

var requestPager = new MyRequestPager([{id: 1, title: 'foo'}]).bootstrap({totalRecords: 50})

I can see what you're saying about this introducing possible overhead into the api since it is an additional explicit function call. In any case you would need to instruct the requestPager as to the total number of pages by some means (of course, if you were already initializing the requestPager with the entirety of it's collection, then you would actually want to be using a clientPager). In the future, the constructor could possibly change to something like this:

var requestPager = new MyRequestPager([{id: 1, title: 'foo'}], {totalRecords: 50})

In this case, the requestPager could implicitly decided that it has a non-empty collection and the totalRecords property specified and the perPage matches the length of the collection (etc.) so that it does not have to make a call out to the server (bootstrap could become a private function in this case). However, this implicit condition could lead to some confusion (maybe not if well documented).

That's the break down. Do you have any ideas for how the interface should work? The author's have been quite reasonable and communicative about change.

@alexandernst alexandernst reopened this Feb 6, 2013
@SBoudrias
Member

The author's have been quite reasonable and communicative about change.

I don't doubt about that. As I said, I've been out of the loop of this project. I switched over to Pageable for more recent project. The API Pageable use is quite simple actually (only one class, and bootstrapping a collection - at least the first page - is implicit).


The issue here was that when setting up a Paginator, you'd put the config inside this.paginator_ui['some option'], but these value where referenced as this['some option'] in every function. The fetch function was then extending this with this.paginator_ui, this is what I meant back then by bootstrapping/initializing the defaults values. This meant that calling any other function before calling fetch was breaking code.

My fix at this moment (from memory) was only:

Backbone.requestPager.extend({
  initialize: function() {
    _.extend(this, this.paginator_ui);
  }
});

Doing this and setting the first_page as 2 was simply bootstrapping the app with initial data without any overhead.

@willsu
Contributor
willsu commented Feb 6, 2013

@SBoudrias, I agree that the references to this.paginator_ui should be cleaned up and standardized. Either the initialization of these properties (as you have done) should happen in the constructor or the references should change in the other functions to this.paginiator_ui[property].

The API Pageable use is quite simple actually (only one class, and bootstrapping a collection - at least the first page - is implicit).

It wouldn't be possible for a paginator to build an accurate pagination state when it is only bootstrapped with the first page of data (and no other information). It would need to know totalRecords (or equivalent) in order to know if there is a next page, previous page, etc.

I imagine that the Pageable library lazily populates it's state after it receives it's first response from the server (which would presumably contain the totalRecords value). This would mean that if you instruct the paginator to fetch the next page, it would have to make a request to the server even if there was no next page of data. However, I'd imagine that you protect against this case in your UI by rendering a disable 'next' link if there is only only page. Does this sound right? I could see this as an alternative approach to bootstrapping. However, if you want to use client side template rendering you couldn't use your javascript object, as it would not have the necessary data that it needs.

@wyuenho
Member
wyuenho commented Feb 14, 2013

State initialization is done eagerly in Pageable. Bootstrapping is fully supported by Backbone.PageableCollection using the exact same interface as Backbone.Collection's. The whole reason we are talking about this and #131, #78 and #124 is because Backbone.Paginator decided to break compatibility with Backbone.Collection right from the start. Pageable doesn't have this problem.

totalRecords is a required state when initializing a server mode PageableCollection. If not supplied, totalRecords is assumed to be the length of the bootstrapped list of models, i.e. the number of models on the first page for server mode. This is done so that PageableCollection has a 100% compatible interface with Backbone.Collection by default. Of course, you can also update the state on the next fetch as well.

@willsu
Contributor
willsu commented Feb 14, 2013

totalRecords is a required state when initializing a server mode PageableCollection. If not supplied, totalRecords is assumed to be the length of the bootstrapped list of models, i.e. the number of models on the first page for server mode.

Thank you, this makes sense.totalRecords must either be specified or inferred as the total length of the collection.

This is done so that PageableCollection has a 100% compatible interface with Backbone.Collection by default. Of course, you can also update the state on the next fetch as well.

I agree that this is a good interface decision. The requestPager should probably degrade to using the size of the collection as well, in the interest of maintaining interface compatibility with Backbone.Collection. However, what would be an actual use case for initializing a requestPager with the entirety of it's data (i.e. if there is to be no communication with the server, then why not just use a clientPager)? I guess I could see a developer wanting this functionality if the data for a given resource is changing rapidly on the server side (i.e. A page was loaded with a collection of length 60 and a per_page of 20, and when the requestPager moves to page 3 a request is made to the server it finds 2 new pages of data, etc). I'd imagine the trick here would be to make requests to the server when the collection has run up against a pagination boundary to discover more (or less) data. I'm just trying to contrive an example so that I can understand :). Regardless, maintaining compatibly interfaces is a good idea, imho.

By looking at the Backbone.Pagable documentation, It would appear that to initialize a PageableCollection and then set the totalRecords (i.e. bootstrapping the PagableCollection with only the first page of data), one would have to write the following code:

// forgive me if this doesn't actually work.. just providing an example here.
var books = new Books([{id: 1, title: 'foo'}], {state: {totalRecords: 100}});

This interface is good because the Books PageableCollection can be bootstrapped as a one-liner. This bears a striking resemblance to an interface that I proposed above:

var requestPager = new MyRequestPager([{id: 1, title: 'foo'}], {totalRecords: 50})

However, it currently works as a one-liner but with a call to the bootstrap method.

var requestPager = new MyRequestPager([{id: 1, title: 'foo'}]).bootstrap({totalRecords: 50})

I am in favor removing the explicit call to bootstrap if the authors see it fit. The code that the bootstrap method executes could be inferred in the object constructor based on the totalRecords field (as I'm sure it is in your project), it would just take a little bit more work.

Please let me know if I missed anything here :).

@wyuenho
Member
wyuenho commented Feb 14, 2013

@willsu You will never bootstrap the entire collection under server mode under any circumstances. The only reason no error is thrown if totalRecords is not supplied under server mode is to pass Backbone.Collection's tests. In reality, you will only ever bootstrap 1 page of models and set totalRecords in the constructor under server mode. No clutchy bootstrap method is required. Everything else works as you described, internal state will get updated on every get*Page method call, which eventually delegate to fetch. So even if your server gets 1 more page when you page to page 2, it'll be immediately reflected on the client side.

You should give backbone-pageable a try, it's really Backbone.Paginator 2.0 :)

@SBoudrias
Member

In fact, total record is only really useful in a view with pagination context. Considering an infinite scroll or any persistent data loading without interface representation of the underlying pagination, setting total record is somehow unnecessary.

For example, via Pageable, I set up the current page in the state, and then I just bootstrap the collection without totalRecords.

var Elements = PageableCollection.extend({

        model: Backbone.Model,
        state: {
            currentPage: 1,
            pageSize: 15
        }
});

var elements = new Elements([ /* Data */ ]);

At this point, when calling getNextPage, we just get the next page (page 2) and total records get updated from the server.

I really haven't played much with Pageable yet; this is really early prototype for a project. So there may be some caveats, but right now this seems like a pretty simple interface working right out of the box (or being just dumb enough to follow it's configuration without asking question - which is somehow good in this case).

@wyuenho
Member
wyuenho commented Feb 14, 2013

I hadn't quite expected that use case, but yep that works too!

@willsu
Contributor
willsu commented Feb 14, 2013

@SBoudrias

In fact, total record is only really useful in a view with pagination context.

Yes, but this is precisely the reason why a developer would use this library. If your use case is only ever fetching the 'next' page of data, then using a fully fledged pagination solution is most likely not worth implementing (of course that's up to you, but you should consider the more common use cases of this library). If you're not taking advantage of the feature set nextPage, prevPage, gotoPage, rendering pagination controls to the UI, etc., then this library is probably not worth using. I'm only taking this stance because it would be an extra 2 or 3 tiny additional lines of code to maintain and increment a page variable in your class extending Backbone.Collection.

Although, yes I agree that the requestPaginator could be designed to work without a pagination context if the authors felt it were necessary. For completeness, it may be nice for the requestPaginator to cover this case.

@wyuenho

You will never bootstrap the entire collection under server mode under any circumstances.

So you agree that the implicit totalRecords setting based on the collection length is purely a Backbone.collection compatibility issue then. Makes sense.

The only reason no error is thrown if totalRecords is not supplied under server mode is to pass Backbone.Collection's tests

I like the idea of running the Backbone.Collection test suite against your Pageable data structure in order to ensure compatibility.

No clutchy bootstrap method is required.

I agree that the bootstrap function should have to be required and have proposed the following interface several times ITT:

new MyRequestPager([{id: 1, title: 'foo'}], {totalRecords: 50})

Which is arguable less 'klutchy' then the Pageable:

new Books([{id: 1, title: 'foo'}], {state: {totalRecords: 100}}); // no 'state' variable required in the example above.

You should give backbone-pageable a try, it's really Backbone.Paginator 2.0 :)

I look forward to trying out your project, as it seems a like there are a lot more interface niceties. However, while we're here maybe we should work on improving this one :).

@addyosmani Any thoughts on this discussion?

@wyuenho
Member
wyuenho commented Feb 15, 2013

Pageable is not just about having a nice interface. The most powerful thing it does is that the origModel in Pageable is a full-blown Backbone.Collection, hence why it's called fullCollection. This decision is crucial in enabling page synchronization across all the pages under client mode and caching under infinite mode. You basically don't have to do any manual state juggling with Pageable under these 2 modes. This is not possible in Paginator without a whole lot of work and breaking a lot of things. Backbone.Paginator is by far the most popular Backbone plugin out there for a number of reasons and it has lots of good ideas, but frankly its implementation is quite broken as indicated by the tickets of this project.

(Pipe dream)

Backbone.Paginator 2.0 = Backbone.Paginator < 1.0 + Backbone.Pageable 1.1.5+

@addyosmani
Member

@wyuenho

To be completely honest, I would be more than happy for the team working on paginator and pageable to collaborate on a Paginator 2.0 which is based on both projects. Right now I think both contain some really interesting ideas and at the end of the day, I care most about offering developers the more flexible solution. I've been intending on moving this project into an organisation - if you (and others interested in working on this) would be up for it we could certainly collaborate.

@wyuenho
Member
wyuenho commented May 18, 2013

I'd love to, but at this point I think the only advantage of merging the two projects for me is that backbone-pageable will get better visibility for obvious reasons. I'm not sure what else in backbone-pageable needs to be improved. It's essentially feature-complete and its bug count is currently at 0. If anything, it'd be just some really small tweaks once in a blue moon when Backbone updates. Besides, Backgrid has a really tight integration with backbone-pageable and I'm not about to break anything.

Frankly I think backbone-pageable supersedes Backbone.Paginator in terms of basic functionality and quality. There are superfluous things in Backbone.Paginator such as filters and the diacritic plugin that I just don't think they belong in a pageable collection. backbone-pageable IS Backbone.Paginator 2.0. If you and the Backbone.Paginator community is willing to bless it as such, I'd be happy to transfer backbone-pageable to the new organization.

@alexandernst
Member

Saying that diacritic/filter plugins don't belong to a client-side pageable solution is just wrong. While filtering/searching on the server side is OK and you can say "I don't care about filtering/searching as that is server side problem", you can't just say "I don't care about this whenever it's server or client side".
Of course, filter/search plugins could be stripped off, but why? What's the need of creating yet another library to include and yet more code to handle (somehow) the whole filtering/searching when we can have it as a plugin in the paging library itself?

@wyuenho
Member
wyuenho commented May 19, 2013

I'm not sure this ticket is the appropriate place to continue this discussion. Please open a ticket on either project and @ my handle and we'll continue from there.

@addyosmani
Member

Agreed. We'll start a new thread to discuss this further. Thanks for your
interest in talking through the idea.
On 19 May 2013 15:16, "Jimmy Yuen Ho Wong" notifications@github.com wrote:

I'm not sure this ticket is the appropriate place to continue this
discussion. Please open a ticket on either project and @ my handle and
we'll continue from there.


Reply to this email directly or view it on GitHubhttps://github.com/addyosmani/backbone.paginator/issues/99#issuecomment-18118321
.

@wyuenho
Member
wyuenho commented May 19, 2013

FYI I'm in the middle of opening a new ticket here. Give me a few minutes....

@wyuenho wyuenho referenced this issue May 19, 2013
Closed

Merge backbone.paginator and backbone-pageable #174

4 of 4 tasks complete
@addyosmani
Member

Sounds good!
On 19 May 2013 15:57, "Jimmy Yuen Ho Wong" notifications@github.com wrote:

FYI I'm in the middle of opening a new ticket here. Give me a few
minutes....


Reply to this email directly or view it on GitHubhttps://github.com/addyosmani/backbone.paginator/issues/99#issuecomment-18118866
.

@addyosmani addyosmani closed this May 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment