Skip to content
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

Proposal: API Calls and FHIR #13

Merged
merged 1 commit into from Sep 7, 2019

Conversation

@joeldenning
Copy link
Collaborator

commented Aug 2, 2019

This proposes a way of making api calls from browser to server.

the OpenMRS-specific APIs will be used.

## Definition
- `fhir` refers to the [official fhir.js npm package](https://github.com/FHIR/fhir.js/). [FHIR itself](https://www.hl7.org/fhir/overview.html)

This comment has been minimized.

Copy link
@gregoryjschmidt

gregoryjschmidt Aug 3, 2019

Have we considered using the java based HAPI FHIR APIs by James Agnew & team, https://github.com/jamesagnew/hapi-fhir
Their project seems to be more active than the fhir.js one

This comment has been minimized.

Copy link
@angshu

angshu Aug 4, 2019

yes. We should evaluate which library to use. However, cautioning again on the common model, which i am sure will be difficult to agree to! (e.g. Patient attributes, would probably need specific extensions, or relationships etc)
I am hoping that we use "Adapters"/"proxies" for FHIR as discussed before. So if the current patient fhir model (provided by openmrs), I should have ability to override that and fetch from a different api (as long as we agree to return a min common agreed model).

This comment has been minimized.

Copy link
@joeldenning

joeldenning Aug 4, 2019

Author Collaborator

@gregoryjschmidt Java doesn't work in the browser. Since HAPI is a java library, we can't use it in the browser. Also, HAPI is for integrating with your database, which is not a frontend concern.

The decision about HAPI is one for the backend, not the frontend.

Also, I did research into fhir.js and smart on fhir client-js. They are both well maintained. Their code is good. It has been updated recently. I'm not sure why you are saying fhir.js isn't well maintained, because it seems well maintained to me. I've been impressed with the project in the research and experimentation that I did, including installing the library and trying it out.

This comment has been minimized.

Copy link
@joeldenning

joeldenning Aug 4, 2019

Author Collaborator

@angshu without a common model it's impossible to prevent duplicate network requests. I very much agree with you that the common model is troublesome and won't work for many use cases. For the code that doesn't want to use the common model, they can use authenticatedFetch or fhir to call whichever endpoint they want to however they want to (instead of getCurrentPatient which provides only the common model). Doing that causes duplicate network requests, but that is okay sometimes.

### `authenticatedFetch`

Maintaining an OpenMRS-specific wrapper around `window.fetch()` allows us to make a single code change when
improving or fixing authentication and other API behavior.

This comment has been minimized.

Copy link
@fatmali

fatmali Aug 6, 2019

@joeldenning Not sure how this would work out, given that zone.js already has wrapped window.fetch()

This comment has been minimized.

Copy link
@joeldenning

joeldenning Aug 6, 2019

Author Collaborator

Since authenticated fetch calls window.fetch, zone.js will still be able to make the fetch promise a “Zone Aware promise” and trigger an Angular re-render when the fetch result comes back.

For angular applications, it might be best to inject our own HttpProvider that calls authenticated fetch instead of XHR or window.fetch. See “common practices section” where that is called out.

@joeldenning

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 7, 2019

Merging this because it represents the reality of our code at https://github.com/openmrs/openmrs-esm-api/. We were unable to drive the conversation here on github to get approvals / discussion, so we're just moving forward with it.

@joeldenning joeldenning merged commit d257868 into master Sep 7, 2019

@joeldenning joeldenning deleted the api-calls branch Sep 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.