-
Notifications
You must be signed in to change notification settings - Fork 12
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
Feature/#25 pagination #44
Conversation
Example usage
|
src/index.js
Outdated
@@ -50,6 +51,108 @@ class AEMHeadless { | |||
this.fetch = this.__getFetch(config.fetch) | |||
} | |||
|
|||
buildQuery (model, itemQuery, args) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that we have QueryBuilder, should we create another flavor of runQuery
that leverages QB instead of passing the whole query?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
runPaginatedQuery
is exactly this.
I've covered all types:
- List
- Paginated
- byPath
The only overhead is that even if you don't want pagination, you still have to call .next()
.
const pathQuery = await aemHeadlessClient.runPaginatedQuery(model, fields, { _path: '/content/dam/wknd-shared/en/magazine/alaska-adventure/alaskan-adventures' })
const result = pathQuery.next()
const offsetQuery = await aemHeadlessClient.runPaginatedQuery(model, fields, { limit: 3 })
const { value } = await offsetQuery.next();
It's easy to add another flavour of runQuery
, it will be just a method which calls runPaginatedQuery
and also executes .next()
in it.
The biggest problem here is to come up with a name for it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I can see how it could be done currently, without runPaginatedQuery
:
const qbResult = client.buildQuery(model, fields, args)
const response = await client.runQuery(qbResult.query)
is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it can be used like this also
* @property {QueryString} query - Query string | ||
*/ | ||
|
||
module.exports = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's great to have jsdoc types. Would be great to add also a d.ts types file for TS projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in separate PR: #45
It's generated from JSDocs
Although TS already support JSDocs same as .d.ts.
} | ||
// Cursor based: Loop all pages | ||
const cursorQueryAll = await aemHeadlessClient.runPaginatedQuery(model, fields, { first: 4 }) | ||
for await (let value of cursorQueryAll) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for my understanding: This pattern uses the same approach under the hood than the one using done
/next()
.
Or, in other words: this pattern does not load everything before entering the loop, it uses lazy loading whenever there are not enough items (aka the next page) is read.
Otherwise it would be counterproductive to what we're trying to achieve with paging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It uses exactly the same code/approach as done/next,
const cursorQueryAll = await aemHeadlessClient.runPaginatedQuery
is not making any requests.
Requests are in for loop
for each page.
src/utils/GraphQLQueryBuilder.js
Outdated
* @param {ModelArgs} [args={}] - Query arguments | ||
* @returns {QueryBuilderResult} - object with The GraphQL query string and type | ||
*/ | ||
const graphQLQueryBuilder = (model, config = {}, fields, args = {}) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as config
and args
are optional, can they come at the end?
my preferred function signature would be:
const graphQLQueryBuilder = (model, fields, config = {}, args = {})
And same for buildQuery
. I'm the kind of lazy dev and if I don't want the optional params, I would just call buildQuery(model, fields)
😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the same at first. Here are the reason why I selected this order
- model and config are both configuration related, while fields and args are strictly GraphQL query related.
- While
config
is optional, it's using default 10 items page size, and it would be good for devs to know that they are choosing default behaviour with providing null to config. - I would also expect that in real usage
{ pageSize }
will always be part of config
But again this are minor reasons, if you still prefer the change I'll add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it work if fields
is an empty string? will it then return some default fields?
If that's possible, I would keep the signature as is and make fields optional as well.
If that's not, I would move fields to next to model. Because fields
is mandatory anyway, I see little distinction between configuration and GraphQL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Order changed.
I can't really go into the details, as I'm not very familiar with "modern JavaScript", but the general usage patterns look pretty good to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. thanks @easingthemes
Closes #25