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

Pagination #1517

Closed
wants to merge 20 commits into from
Closed

Pagination #1517

wants to merge 20 commits into from

Conversation

simonexmachina
Copy link
Contributor

This PR adds the foundation for pagination support in Ember Data. Example usage using an 'infinite-scroll' loadMore approach can be seen here. The goal of this PR is to provide the underlying requirements to support pagination, so that the community can iterate on rest of the implementation in userland until we've got something we're happy with.

The solution adds a DS.Request class to contain information about the parameters that were used to create a given AdapterPopulatedArray, thereby allowing for more pages of results to be fetched from the server.

I've folded the sinceToken into the Request class, but this is the only 'breaking' change. So Adapter.findAll: function(store, type, sinceToken) is now Adapter.findAll: function(store, type, request).

Please feel free to offer any feedback - I've tried to stay close to the style of the rest of the project, but happy to take on board any feedback.

One possible concern is that I'm using page and pageSize parameters which is nice and simple but there's an edge case where the pageSize changes and leads to duplicate/missing records in the series of pages. I'd prefer to use offset and limit, but the common practice seems to be page and pageSize so I've used this approach instead.

Todo

  • Currently only findQuery() and findAll() support pagination, I'll add support for the other find...() methods now

Added DS.Request class for pagination, sinceToken etc
Added documentation
Store any metadataFor(type) in the created DS.AdapterPopulatedRecordArray
Added pagination support to findAll()
Added ability to specify the request param to use for `page` and `pageSize`
@simonexmachina
Copy link
Contributor Author

I've just noticed the source for AdapterPopulatedRecordArray refers to it being immutable, but I'm mutating this array to provide the pagination. Is this a problem?

* upstream/master:
  Latest ember-dev uses local packages.
  [BUGFIX beta] Prevent error in failed reload().
* master:
  Latest ember-dev uses local packages.
  Fixed JSONSerializer.extractArray()
  [BUGFIX beta] Prevent error in failed reload().

Conflicts:
	packages/ember-data/lib/serializers/json_serializer.js
@simonexmachina
Copy link
Contributor Author

Tests are complete (AFAICT), looking forward to some feedback. Bring it on!

@dschmidt
Copy link

Not knowing much of ember (data) internals, I'm wondering whether it will be possible - with your work merged - to do pagination with links.

What I have in my User response right now:
links: { posts: "/users/$id/posts" }. Currently I can't seem to find a way to fetch more items on demand here.

Will this PR help with that?

@simonexmachina
Copy link
Contributor Author

Yes it will. My plan is to publish an ember-pagination module that you can use to do this. It’ll include PagingRouteMixin and Paginator classes to help make that use case really easy.

My hope is that the community will iterate on the best solution at the UI end and we’ll get something that’s ready to move into core.

@OpakAlex
Copy link

How we can get size of pages?
And how I can pagination one resource in 2 place on page?
For example I have some posts in menu 'programming' and some posts in menu 'design' and I want paginate 'post' in 2 place on 1 page. Can i do this with your PR?

@simonexmachina
Copy link
Contributor Author

How we can get size of pages?

The adapter can specify the page size, this is controlled by it's pageSize parameter: store.adapterFor('post').get('pageSize')

I've just added a binding in AdapterPopulatedRecordArray so you can get it from the result like so:

store.findAll('post').then(function(array) {
  array.get('pageSize');
});

It's also possible for the pagination to be enforced from the server side, in which case this can be found in array.meta. I haven't added an abstraction to handle servers using different property names for this (eg. per_page vs. page_size etc), but this could be done. I'm not sure it's necessary though, thoughts?

And how I can pagination one resource in 2 place on page?

You just need to have two RecordArrays, they will each have their own page number and you can call loadMore() and loadPage() independently on each.

@dschmidt
Copy link

Can you add records to a not-completely loaded relationship? And can you persist that to the server?

Added pageSize, total, totalPages and isFinished to AdapterPopulatedRecordArray
Added normalizeMetadata() to JSONSerializer to provide standardised access to page, pageSize, total, isFinished
Added pagination tests
metaForType() object is now an Ember.Object so it is observable
Refactored for separation of concerns in Request and AdapterPopulatedRecordArray
Changed AdapterPopulatedRecordArray.page to startPage and endPage
* upstream/master:
  [DOC] Stop store.pushPayload docs overwriting store.push
@simonexmachina
Copy link
Contributor Author

I've thrown together the beginnings of a module that shows how this PR can be used to build the UI. It provides 'infinite-scroll' style pagination and also standard page-based navigation as well.

https://github.com/aexmachina/ember-pager

You can see a demo here: http://jsbin.com/ujaWImap/latest

@simonexmachina
Copy link
Contributor Author

@wycats and @bradleypriest, not really getting any feedback on this - is there something that I should be doing to get this moving forward?

I think it's important that we get this in before 1.0 - it'd be a real shame for 1.0 to go out without decent support for pagination.

@bradleypriest
Copy link
Member

Hey @aexmachina, love what you're doing here, I haven't had much time to check through it at the moment, but I'll try and have a look this week sometime.
Would love to replace the custom pagination I'm using

However, I personally am kind of against putting this in before 1.0.

Flexible pagination is hard, and I'd rather we manage this as a separate plugin being used in production apps for a couple of cycles first.
Iron out the kinks before we merge it into core.
Sound reasonable?

@simonexmachina
Copy link
Contributor Author

Hi @bradleypriest, thanks for getting back to me. What I'm trying to do here is to add the absolute minimum that's required in core, in order to allow the rest to be implemented using a plugin.

Essentially all this PR does is to add support for loadMore() and loadPage(), as well as page number, pageSize and total properties. I think this is the bare minimum, and IMHO I think this is probably all that should be provided by Ember Data - everything else can be provided by plugins.

Without this core support in Ember Data I think any plugin is always going to having to do heavy lifting to make this work, and we're not providing the community with clear conventions for how to represent this information. However, with the core features in this PR it become pretty simple to provide pagination support on top of Ember Data. I've pushed an ember-pager plugin that shows one way that this could be used, and you can see a JSBin example here. But I'm sure there's lots of other ways this could be done, and I don't claim to know which one is the 'best'.

I personally think that this really does need to be in 1.0, and I think that this PR provides a very minimal and non-controversial implementation that allows the community to work through the details, providing the 'conventions' that then allow the community to build on top of. Really looking forward to hearing your thoughts :)

@lukemelia
Copy link
Member

The minimal and controversial support is the support for meta, which already exists, and this PR is indeed building on top of it. This PR creates more extra machinery/concepts than I would hope is necessary to support pagination (e.g. DS.Request).

I would concur with @bradleypriest on this. Let's make an ember-data specific library that can add conventional pagination. If there are ember-data APIs that make it impossible to do that, let's discuss what those are.

@mahasamatman
Copy link

Here is how i rewrite (https://github.com/VisualGuruz/emberjs-pageable) one existing mixin:

https://gist.github.com/mahasamatman/b58622b51364c87f7814

It completely independent from Ember-Data (except metadata properties, but there are several methods to load it). The only thing you should do - is implement getPage(page) action somwhere. Simple? Yes:)

@simonexmachina
Copy link
Contributor Author

I would concur with @bradleypriest on this. Let's make an ember-data specific library that can add conventional pagination. If there are ember-data APIs that make it impossible to do that, let's discuss what those are.

I totally agree here. That's precisely what I'm trying to do - I accept that I may be overreaching with the addition of loadPage/More() to the record array. If we decide that we don't want to provide this, then I'd guess that we can do it without DS.Request. However, it should be said that think this is the right solution, and I think that DS.Request is a perfectly reasonable addition.

This PR creates more extra machinery/concepts than I would hope is necessary to support pagination (e.g. DS.Request)

The Request class is used to contain the information that was used to create the RecordArray, and it's what allows us to provide the loadPage() and loadMore() methods. The alternative is to handle this at the application level (eg. in the Route), but I much prefer this being provided by Ember Data.

@mahasamatman, that's a really nice solution. The main difference if this PR was accepted is that the getPage() method doesn't need to be provided by the user and can be handled entirely by the adapter. Also, the didLoadContent() method is dependant on the server using meta.total_count, but with this PR that can be handled by the adapter - so you could plug it in to different backends that use a different property name for this. I've forked your gist to show how it would look with this PR, and both those drawbacks are eliminated.

The main benefits of this PR can be summarised as follows:

  • The metadata provided by the server-side can be normalised, so that plugins don't have to deal with these details (eg. meta.total_count, meta.total or whatever can be normalised by the adapter)
  • pageSize can be controlled by the server side, whether it's specified by the client or not
  • Pagination can be supported by the FixtureAdapter (important for testing)
  • The parameter names that are required by the server-side (eg. pageSize vs per_page) can be handled by the adapter (rather than throughout the application code)

And we also get loadMore() and loadPage() which I think are cool :)

I'd be open to doing this entirely in a plugin, but I can't see how that could be done using what's currently supported.

Basically, if this PR goes too far then we can remove loadPage/More() to get rid of DS.Request, but I think this would be a shame. However, if we don't provide solutions to the above and don't have some basic support for pagination built-in then we're sending out Ember Data half-baked at version 1.0, which would be a real shame because it's otherwise a wonderful solution.

@huafu
Copy link
Contributor

huafu commented Nov 28, 2013

Some work I've made related to infinite scroll, search filters and pagination, not depending on any new thing in ED, working with ember-list. I posted here since I thought it might help/inspire/whatever: https://gist.github.com/huafu/6984862

resolver = Ember.RSVP.defer();
request = adapter.requestFor(this, type, null, page);

request.fetchPage = function(store, type, query, page) {
Copy link
Member

Choose a reason for hiding this comment

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

rather then placing this on the object, it seems like it should be an argument/option to requestFor and passed to the request constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, that's good feedback. Will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have done.

Several improvements based on feedback from stefanpenner
@simonexmachina
Copy link
Contributor Author

@stefanpenner I've made a bunch of changes based on your feedback (thanks!) When would be a good time for us to have a chat with the developer you mentioned would be worth chatting to? Really hoping you can continue to help me push this forward :)

@recipher
Copy link

Is this dead?

@simonexmachina
Copy link
Contributor Author

@recipher I'm willing, but I haven't been able to get agreement from the core team on moving this forward. I just don't think that it's a priority for them at this stage. @wagenet has indicated that he's open to a discussion on the forum, so I'll try to restart discussion over there...

@recipher
Copy link

That'd be great. This would have made my work so much easier on the thing
I'm working on at the moment, but I've had to work around it.

Does this work on relationships? Something like...

this.get('blog.comments').loadMore();

?

On 13 June 2014 00:42, Simon Wade notifications@github.com wrote:

@recipher https://github.com/recipher I'm willing, but I haven't been
able to get agreement from the core team on moving this forward. I just
don't think that it's a priority for them at this stage. @wagenet
https://github.com/wagenet has indicated
#1059 (comment) that
he's open to a discussion on the forum, so I'll try to restart discussion
over there...


Reply to this email directly or view it on GitHub
#1517 (comment).

@koemeet
Copy link
Contributor

koemeet commented Oct 17, 2014

Also issue #1937 is a common use case. You only see examples with the find() method. But you should also be able to query async hasMany relations (through the get method). See the example in the issue. What do you guys think about supporting this?

@igorT
Copy link
Member

igorT commented Jun 8, 2015

Just talked to @aexmachina, we will be using the new JSONApi meta for pagination, so closing this PR.
@wecc this PR would be good to review before starting work on pagination

@oligriffiths
Copy link

@aexmachina What's the state of play with this?

@simonexmachina
Copy link
Contributor Author

Long stale by now :)

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

Successfully merging this pull request may close these issues.