feat(graphql): add optional cursor pagination to GraphQL backend service

[Harsgalt86]

==== Purpose of this PR ===
A PR to implement cursor based pagination (by chunks).
Currently slickgrid cursor based pagination only supports first and so when building the query ends up retrieving A-B, A-C, A-D...A-N.
This PR adds implementation for last, after and before
(This is in contrast to the documentation on the wiki that acknowledges it not being fully supported, but has an example of using after which isn't respected)

We can then use these chunk results to create pages

• page1: {startCursor: A, endCursor: B }
• page2: {startCursor: B, endCursor: C }
• page3: {startCursor: C, endCursor: D }

This is in contrast to relay-style pagination which is better suited for realtime and infinite scrolling

• page1: {startCursor: A, endCursor: B }
• page2: {startCursor: A, endCursor: C }
• page3: {startCursor: A, endCursor: D }

=== Behavioural Subtlties ===
Cursor pagination is conceptually similar to a linked list, while offset pagination is similar to a Vector.
As such "goToPage(x)" is not supported when using a backend service configured to use cursors.
In this case, the page-number input is rendered as a span instead. (See slick-pagination.component)

=== Usage ===
Along with everything documented on
https://github.com/ghiscoding/slickgrid-universal/wiki/GraphQL
The pagination-service exposes a new function updatePageInfo which accepts an updated PageInfo object, as part of the graphql return result.
This currently needs to be manually by the application called as part of the process(query) callback. In the future it might be possible in (looking at Aurelia-Slickgrid library) that this call can be made internally in that library automatically.

The pagination-service automatically determines if it the associated backendService is using cursorBased pagination.

=== Future work ===
It would be nice to possibly support relay-style pagination in slickgrid that given when scrolling and reaching the end of the table, it allows automatic fetching and appending of the next page of results rather than actual number paging. Essientially implementing a tabular basd news feed.

[codecov]

Codecov Report

Merging #1153 (3805d79) into master (bd132c5) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #1153   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          245       245           
  Lines        17166     17211   +45     
  Branches      6196      6227   +31     
=========================================
+ Hits         17166     17211   +45     

Files	Coverage Δ
packages/common/src/interfaces/index.ts	100.00% <100.00%> (ø)
packages/common/src/services/pagination.service.ts	100.00% <100.00%> (ø)
packages/graphql/src/services/graphql.service.ts	100.00% <100.00%> (ø)
...nation-component/src/slick-pagination.component.ts	100.00% <100.00%> (ø)

[ghiscoding]

Thanks for the PR, I'll have a deeper look in the weekend and do a review at that time. However from a quick glance at it, I see that you added a lot of cursor based code in the Pagination Service, is it essential though? I mean other than GraphQL, I'm not sure that other backend services, like OData, will need this and that adds a lot of potential not essential code to these other services (again like OData). If you think this is really the only way to deal with this, I could understand but I'm just asking as a general questions because I'm trying to keep these backend pagination services as generic as possible. I see you added tests, which is great but it looks like 4 lines are missing coverage, that would be really great if you can cover them or if you really can't then worst case we could add some istanbul skip, I really want to keep my 100% coverage since it took me a long time to get to that point and I work to keep it that way 😉

Oh maybe another thing, I also have Example 10 in here for GraphQL, it's only the query string that I check but if there's a way to somehow add some quick E2E tests that would be great, we could also add another example if it's too hard to deal with current one. Because it's nice to see it live in action too and make sure that the pagination works as expected. Also what happened when you start filtering? I mean typically when I filter, I generally reset and go back to first page, I assume that would be the same use case with GraphQL cursor based, correct? Anyway, that's why E2E testing would be nice too I guess

[Harsgalt86]

I see that you added a lot of cursor based code in the Pagination Service, is it essential though?

Yeah, I think it does need to be "cursor aware" primarily because cursor based pagination doesn't support the goToPage(x) paradigm. Because of this, the pagination component also needs to avoid rendering an input that allows the user to jump to page X, so at the very least that component needs to know whether the underlying service is configured to use cursor pagination (In this case exposed via getter on the pagination service directly to the backend service)

I'm trying to keep these backend pagination services as generic as possible

Agreed, in fact, keeping it generic was my intent too. My rationale was that GraphQL is currently the only cursor based service I'm aware of, but it's forseeable another backend service in the future may implement the cursor based paradigm.
Hence the processOnPageChanged signature provides optional cursorArguments if the underlying service supports it, ignoring it if doesn't.

My initial approach here was to create two interfaces OffsetPaginatedBackendService and CursorBasedPaginatedBackendService which OData would only implement the former, while graphQL would implement both, but I believe I was able to achieve the same more succinctly by providing cursorArgs to the processOnPageChanged function.

I mean other than GraphQL, I'm not sure that other backend services, like OData, will need this and that adds a lot of potential not essential code

I think I've handled that, the OData backend service has no associated changes (it is only aware of the original processOnPaginationChanged structure)

I see you added tests, which is great but it looks like 4 lines are missing coverage, that would be really great if you can cover them

Done (Awesome having them, wish my project had such extensive tests :))

Oh maybe another thing, I also have Example 10 in here for GraphQL

Yup I can take a look into doing that

Also what happened when you start filtering? I mean typically when I filter, I go back to first page, I assume that would be the same use case with GraphQL cursor based, correct?

Yeah, good point, this is my expectation too. I might be misremembering but I recall when developing that I saw when the page size changes it goes back to the first page via use of defaultPaginationOptions. I created defaultCursorPaginatiopnOptions to do the same thing, but I need to double check the implementation - and provide a unit test for it too

[ghiscoding]

@Harsgalt86 I reviewed and left a few comments but they are very minor, really good job. I would be really interested to see this in action, so yeah modifying or adding a new example would be really nice to see it live. Were you actually able to test this with an actual live GraphQL server? Also in continuation with the last comment I wrote with filtering and going back to page 1, have you tested with sorting as well? How would that work, would it resort from current cursor position and we drop end cursor because it probably changed?

If you want to add a new Example, that is if it's too complicate to modify Example 10, you can take a look at this commit b7e9e0d where I added Example 19 recently. There's purposely no framework in here but I've added some very basic binding services, it's similar to Aurelia with some minor diff like onclick.delegate which has an extra on in the front, I guess you can see where I got the inspiration from haha. However, I think you can probably modify Example 10 to achieve this, we've done something similar with Example 9 (OData) where there's a bunch of radio box and checkbox to test different feature, so you can probably do that for the GraphQL example too.

Also note that I'm trying to finish implementing sub-menus for all Menu plugins (I have 3/4 done) and I expect to probably be able to release sometime early next week if I can finish that and other tasks that I have identified. It would be great to include your PR as a new feature for this next release.

[Harsgalt86]

@ghiscoding Thanks for the quick turnaround on the review. I've implemented your suggestions except for the demo stuff which I'll look into doing this week.

Were you actually able to test this with an actual live GraphQL server? Also in continuation with the last comment I wrote with filtering and going back to page 1, have you tested with sorting as well?

Yes, our server already supported cursor based pagination, but is in the process of supporting the sortBy/filterBy syntax so will get will work through that and get back to you.

How would that work, would it resort from current cursor position and we drop end cursor because it probably changed?

On a filter/sort parameter change, we would need to go back to the first page {first: 20} because, current pageInfo will be stale.

eg: Say you on page 2 and make a change to the filters which makes the first page and its 20 items filtered out. The current pageInfo (eg: hasPreviousPage) is out of date and the pagination needs to be updated the first available 20 items becomes page 1.

It would be great to include your PR as a new feature for this next release.

Sounds good! I'll do what I can.

[ghiscoding]

@Harsgalt86 I took a look at the update Example 10 and tested locally and I noticed something that I'm unsure if it's ok or not.

When using cursor, if we change page then only after gets changed which is great

but if we change filter (for example on gender), then after is reset to empty string but we now also have 2 extra props before and last which are both set to undefined. You can already guess my question... is that intended?

apart from that, the rest looks ok I think

[Harsgalt86]

@Harsgalt86 I took a look at the update Example 10 and tested locally and I noticed something that I'm unsure if it's ok or not.

When using cursor, if we change page then only after gets changed which is great

but if we change filter (for example on gender), then after is reset to empty string but we now also have 2 extra props before and last which are both set to undefined. You can already guess my question... is that intended?

Yeah, not intended, just pushed a fix for it.

I was playing around with the sorting too, it looks like it works without having to go back to the first page. I guess because the 1) totalCount doesn't change 2) the query is executed immediately and 3) pageInfo is updated immediately after. Seems to be consistent with how the offset behaves too.

[ghiscoding]

great that was quick, so is that ready to be merged then? I might do a release tomorrow or in 2 days max

[Harsgalt86]

so is that ready to be merged then

Yeah, I think it can be. If I come across any bugs I'll just raise fixes for them as they come up.

[ghiscoding]

awesome, let's go ahead and merge :)

[ghiscoding]

@Harsgalt86 I just released a new version for Aurelia-Slickgrid v6.4.0 which includes this and also updated Aurelia-Slickgrid Example 6 to include your changes. I did however added a new .setCursorBased() method (as shown in PR #1171) in the Pagination Service because I wanted to change the pagination cursor without reloading the Aurelia-Slickgrid page like you did in here.

I think the only thing missing would be to add more documentation, would it be possible for you to update the Wiki? I think the Aurelia-Slickgrid should be open to public for editing, if it's not then let me know. I'll update the other libs Wikis afterward. Perhaps this Wiki
https://github.com/ghiscoding/aurelia-slickgrid/wiki/GraphQL-Pagination

Thanks again for your contribution 🚀

[Harsgalt86]

@ghiscoding No worries. Is there any consideration to keeping the paginationService and graphQL service in sync programmatically? Or is it just up to the user to keep both in sync?
It probably doesn't matter too much since in the real world there's no need for it to be changed on the fly and it's already being initialized with the backendService value

I think with your change example10 could also be updated as switching between cursor/offset involves reinitialization of the slickgrid instance. I can update that with the wiki update.

[ghiscoding]

No worries. Is there any consideration to keeping the paginationService and graphQL service in sync programmatically? Or is it just up to the user to keep both in sync?

I'm not exactly sure what you mean... but maybe it's related to the following fact, I think that because at some point in the code it creates a deep copy of the grid options, it also kind of makes a copy of the backend services like GraphQL at least that is what I think is happening and because of that I had to add the setCursorBased in the Pagination Service because it wasn't in sync with GraphQL Service, is that what you meant? Perhaps the problem is that it might not be a good idea to add all these services inside the grid options, I think I should have kept it for simple props and not add service instances like we do with the backend services, it might something to consider in a future release though I'm not sure where else to put them.

To compare with SlickGrid, in the core lib you would do something like the code below, the advantage is that grid options remains as small props like flags and maybe couple of callbacks but mainly just flags. It's a good thing because we can do deep copy and it won't matter much, however in my libs that becomes a problem since I chose to have them in the grid options, the main reason I chose to go that way was mostly because of how we have to deal with Components (Angular/Aurelia/...) and that is not the same as dealing with pure JS approach. Anyway, it's something to think about like I said. I'm pretty that I'm doing some deep copy somewhere and that causes some out of sync.

const dataView = new SlickDataView();
const grid = new SlickGrid('#myGrid', dataView, gridOptions);

// then any services/plugins will have to be registered separately, not inside the grid options
const contextMenu = new SlickContextMenu();
grid.registerPlugin(contextMenu);

For the Example 10 in Slickgrid-Universal, I'm trying to improve it so that it doesn't require disposing the entire component and then query some DOM elements, I'll be adding some PubSub in PaginationService so that when I call setCursorBased, it will recreate the entire pagination component because at this point if you test it in Aurelia-Slickgrid, you will see that it doesn't remove the page input number like what you did in universal. Anyway that is not something that rush, it would be very rare that users will toggle between cursor/offset in the same grid, it's mostly just good for the demo. I don't think it's required for the Wiki since like I said, it would be rare that users would switch on-the-fly between these two.

[Harsgalt86]

it kind of makes a copy of the backend services like GraphQL at least I think that is what is happening and because of that I had to add the setCursorBased in the Pagination Service because it wasn't in sync with GraphQL Service

Yeah, by having it as below, changing the backendServiceApi options would automtically update the paginationService...

get cursorPageInfo() {
   return !!this._backendServiceApi?.options.isWithCursor;
}

but it seems as though maybe this wasn't working if the options were being deep copied.
In which I was thinking of a way to avoid having to call both paginationService.setCursorBased and backendService.isWithCursor .

But yeah a non-issue, really.