Added getResource & list helpers - #1756
PiusKariuki wants to merge 18 commits into
Conversation
|
@hunterachieng this now ready to review. See description for summary |
mtuchi
left a comment
There was a problem hiding this comment.
Hey @PiusKariuki can you rebase from main and make sure your branch is up to date and doesn't contain other packages changes eg: msgprah, fhir-opensrp or surveycto
dc855c7 to
c51921d
Compare
|
@mtuchi
Note: Notice that the build is now failing in the GIT status check. This was the reason I included the files above in the PR since this step kept failing. Do you have any recommendations on how to fix this in a cleaner way? See error below |
652f8ae to
9cad415
Compare
| export async function requestWithPagination(configuration, path, options = {}) { | ||
| const { domain, apiVersion = 'v2' } = configuration; | ||
| const { resultsKey } = options; | ||
| const targetUrl = `/a/${domain}/api/${path}/v2`; |
There was a problem hiding this comment.
@PiusKariuki i don't think we should hardcode version /v2, Are you sure all endpoints will be supported ?
what if i want to use v1 ?
There was a problem hiding this comment.
@mtuchi The challenge here is that CommCare v1 and v2 APIs have semantic differences when it comes to pagination.
V1uses the commonlimitandoffsetV2uses acursorto paginate.- The shape of the response is also different depending on what version you are using.
That is the reason I am using version 2 so I can be able to control what shape the response comes in and how I move on to the next batch.
The docs state on that 2 CommCare resources versioned as v1/v2: the Case Data API and the Location API. On other endpoints V2 is just a superset of V1 so anything you can do in v1 you could also do in v2
There was a problem hiding this comment.
I don't understand what you mean by V2 uses a cursor to paginate. but my problem is getResource() will always use v2 because it's hard coded regardless if i specify apiVersion in my configuration. Maybe the that's fine and we should document in the function this function uses v2 only.
If we want to support auto pagination then regardless of v1 or v2 getResource() should support auto pagination and not force the function to only use v2.
There was a problem hiding this comment.
Maybe @josephjclark have different opinion on this
There was a problem hiding this comment.
I think the question here @josephjclark is, how opinionated should the requestWithPagination function be when it comes to the API version ?
The 2 API versions handle pagination very differently and give the response in different shapes so I made the call to use v2.
| /** | ||
| * Options provided to the `getResource()` operation. | ||
| * @typedef {Object} GetResourceOptions | ||
| * @property {Object} [params] - HTTP query parameters passed through to CommCare. |
There was a problem hiding this comment.
CommCare query parameters is clear i think
| const data = await util.requestWithPagination(state.configuration, resolvedResourceType, resolvedOptions); | ||
| const nextState = util.prepareNextState(state, data); | ||
|
|
||
| return { |
There was a problem hiding this comment.
I think we just need to return nextState no need for { ...nextState, data}, thats what prepareNextState do
There was a problem hiding this comment.
@mtuchi I tried it locally and it returned state without a data object
There was a problem hiding this comment.
Hey @PiusKariuki , left couple of comments here, please review them. Also see my comments on CommCare url scheme change from the previous pr 👇🏽
|
@mtuchi please take a quick look at this PR when you can. |
|
@PiusKariuki can you check the box in review checklist |
mtuchi
left a comment
There was a problem hiding this comment.
Hey @PiusKariuki i left you couple, Please address them and ask joe for final review
…Fn/adaptors into commcare-add-fetch-and-list-helpers
|
Hi @josephjclark could you please have a look at this when you get the time? |
|
I can look but I don't see an approve from @mtuchi yet. There are also conflicts because the version was bumped too earlier. Please submit a changeset file with all changes, but don't bump versions until we're ready to merge (usually I'll ask when ready for that) i recommend you revert the package.json and changelog files. |
|
I don't know where there are so many changes in the package lock either - you should probably revert those |
Summary
Added 2 helper functions to
adaptor.js:list&getResourceFixes #1742
Details
requestWithPaginationhelper in util.jslistoperationrequestWithPaginationgetResourceoperationAI Usage
Please disclose how you've used AI in this work (it's cool, we just want to
know!):
You can read more details in our
Responsible AI Policy
Review Checklist
Before merging, the reviewer should check the following items:
production? Is it safe to release?
dev only changes don't need a changeset.
pnpm run versionused to bump versions (notpnpm changeset versiondirectly)? This ensures changelog dates are stamped correctly.