Refactor pagination#58
Conversation
Jammjammjamm
left a comment
Jammjammjamm
left a comment
There was a problem hiding this comment.
This looks like a promising refactor. A few general comments:
- It's not totally clear which methods are intended to be public/private, so
@privateshould be added to the private ones. - The public methods should have examples and tests. I see tests were added for
gotoPage, butinitializeat least seems like another public method that should be tested. - I brought up several naming issues. We should probably just discuss these in person and see what we can come up with.
| * | ||
| * @returns {void} | ||
| */ | ||
| initialize(results) { |
There was a problem hiding this comment.
If results is always a Bundle, I think it should be called bundle.
| * | ||
| * @returns {void} | ||
| */ | ||
| parseUrl(link) { |
There was a problem hiding this comment.
I think this could have a more descriptive name. Something like extractSearchParams or detectSearchParams?
| parseUrl(link) { | ||
| const linkUrl = new URL(link.url); | ||
|
|
||
| Object.keys(this.paramNames).forEach((paramName) => { |
There was a problem hiding this comment.
This looks like it should use Object.entries
Object.entries(this.paramNames).foreach(([paramName, param]) => ...)
| * | ||
| * @return {Promise<Object>} FHIR resources in a FHIR Bundle structure. | ||
| */ | ||
| nextPage() { |
There was a problem hiding this comment.
If nextPage and prevPage take an optional bundle as an argument, they can call initialize with it. That way this PR won't break the existing interface.
| * @param {Object} [paramNames.countParam] server-based result count per page, optional. | ||
| */ | ||
| constructor(httpClient) { | ||
| constructor(httpClient, paramNames = {}) { |
There was a problem hiding this comment.
The client constructor doesn't provide a way to pass paramNames to this constructor.
| this.currentResults = null; | ||
|
|
||
| const defaultParamNames = { | ||
| searchIdParam: '_getpages', |
There was a problem hiding this comment.
Could you explain what this param actually is?
I see:
offsetParam: '_getpageoffset',
countParam: '_count'
and those make obvious sense, but when I look at searchIdParam: '_getpages' I don't really know what's going on.
| * here if needed for variations in server parameter names. | ||
| * | ||
| * @param {HttpClient} httpClient a configured instance of http client. | ||
| * @param {Object} paramNames pagination-related search parameter names, optional. |
There was a problem hiding this comment.
Could we discuss some alternative names for paramNames? Probably easier to do face to face.
| goToPage(page) { | ||
| const selectedPage = this.goToPageLink(page); | ||
|
|
||
| this.currentResults = selectedPage ? this.httpClient.get(selectedPage) : undefined; |
There was a problem hiding this comment.
This is really just getAndSetCurrent(selectedPage), right?
| prevPage(results) { | ||
| const prevLink = results.link.find(link => link.relation.match(/^prev(ious)?$/)); | ||
| return prevLink ? this.httpClient.get(prevLink.url) : undefined; | ||
| getAndSetCurrent(link) { |
There was a problem hiding this comment.
I'd rename this setCurrent. I think it's pretty normal for a setter to return the value that was set, and the fact that it does so doesn't need to be called out in its name. Maybe we could look into using getters and setters or something and come up with a solution that doesn't need set in the name either.
| * | ||
| * @return {String} link to specified page based on regex. | ||
| */ | ||
| getLink(regex) { |
3 participants
The pagination class is refactored here to:
_getpages,_getpagesoffset, and_count).Built to support pagination in FHIRKit Create React App.