-
-
Notifications
You must be signed in to change notification settings - Fork 219
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
feat(#9065): add cht-datasource to support get person by uuid #9090
Conversation
…nabled `composite`.
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.
+1 on what Gareth said about the API design it makes more sense to have the two functions implement the same interface. I wish the TS team had implemented microsoft/TypeScript#420, it would have been pretty nice in our scenario but we can surely make cht-datasource
work without!
shared-libs/cht-datasource/src/remote/libs/remote-environment.ts
Outdated
Show resolved
Hide resolved
…atasource object is a nested hierarchy.
…5_get_contact_by_id
… have app-settings service be injected
# Conflicts: # sentinel/src/lib/purging.js
@m5r @sugat009 This is ready for another review! Actually, IMHO this code is "complete" in that I would be comfortable merging it to
(These other three should be much quicker to add now that the patterns/utils are stabilizing). |
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.
From a testing perspective, this looks good. I just have a small suggestion. Thank you!
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 did a thorough pass of the PR and it looks great! I know you're expecting to add a few more APIs to the Person and Place APIs but the groundwork you laid here is fantastic and ready to merge IMO 👏
I wouldn't mind having the additional APIs in another PR, it would make the changes easier to review
@m5r this should be ready for a final review! 🙏
Good call! Once this PR is approved, I will merge and then we can open the subsequent PRs against |
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! Well done Josh
Description
#9065
cht-datasource
This PR converts
shared-libs/cht-script-api
intoshared-libs/cht-datasource
as a general purpose data-access layer intended for use within cht-core both in server and the client-side code as well as within custom config code for (tasks, targets, etc).There are two modes of functionality exported from
cht-datasource
. Both require you to first acquire aDataContext
(eitherremote
orlocal
depending on if you can interact directly with the CHT api server or if offline functionality is required).Imperative
The imperative mode provides a hierarchical factory object that you can drill down into and interact with the datasource.
This mode is useful for exposing functionality to custom configuration code (tasks, targets, etc) by passing the datasource object directly into the custom function.
Declarative
The declarative mode provides more flexible access to API functions. The
dataContext
must be curried when using the functions.Code review checklist
Compose URLs
If Build CI hasn't passed, these may 404:
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.