search

smart-on-fhir / sample-apps-stu3

Collection of simple sample apps
Other
47 stars 43 forks source link

Medications results ignored after fhirclient 0.1.12 #13

Open hansenms opened 5 years ago

hansenms commented 5 years ago

After release 0.1.12 of fhirclient, fetchAllWithReferences returns a search bundle instead of an arrat of results and consequently this call https://github.com/smart-on-fhir/sample-apps-stu3/blob/master/medications/index.html#L57 is empty and no results are returned. It is unclear if it is actually a bug in the fhirclient or here in the samples repo.

vlad-ignatov commented 5 years ago

It seems to work fine for me with http://launch.smarthealthit.org/v/r3/fhir. Can you provide an example or description of how you launch the medications app (including the patient that you select)?

hansenms commented 5 years ago

I am not quite sure how to get the medications app to work with that launcher, but the problem is that after release 0.1.12 of thr fhirclient, the fetchAllWithReferences returns a search bundle (instead of an array of resources), so the results.length is undefined and consequently no medications are returned. It is the same problem as pointed out in this PR: https://github.com/smart-on-fhir/sample-apps-stu3/pull/12

vlad-ignatov commented 5 years ago

Launching with that server is simple - just run npm start in the medications app and then open http://127.0.0.1:9090/launch.html?launch=eyJhIjoiMSJ9&iss=http%3A%2F%2Flaunch.smarthealthit.org%2Fv%2Fr3%2Ffhir.

Which server are you launching with?

hansenms commented 5 years ago

Thanks @vlad-ignatov. So that should allow you to repro the problem. If you run the medications app locally and use the link you posted above, it will go through the flow but for all patients you get "No medications found for the selected patient". For instance, for patient aae6d4ab-c671-486a-9aa4-ba7ce93e804e the app returns no medications eventhoug there are 2 medications for that patient.

vlad-ignatov commented 5 years ago

Got it. I was testing with 1.0.12.

Unfortunately the change is in fhir.js and I can't revert it without loosing other stuff. I'll have to patch it somehow.

hansenms commented 5 years ago

Could we just lock the version in the affected apps for now. As proposed in https://github.com/smart-on-fhir/sample-apps-stu3/pull/12

vlad-ignatov commented 5 years ago

I think you are right. Patching the client or maintaining a fork of fhir.js should be avoided if possible. I could also "fix" the sample apps. That is not a big deal, but I wonder if any real-world app is affected.

hansenms commented 5 years ago

@vlad-ignatov, do you know if the change to the interface of that function was intentional or not? If it was intentional, the we should fix the app and force the version to be > 0.1.13. Otherwise, we should limit the version to be <0.1.12 until a fix is in and it should be a bug on the client.

vlad-ignatov commented 5 years ago

It was not intentional but it is more complicated than that.

The client depends on fhir.js in a weird way.

  1. fhir.js does not come with pre-built JS. It is not a module that you can require and have it being built with the rest of your code. Instead, you are supposed to build it yourself. Then we require the whole thing from https://github.com/smart-on-fhir/client-js/blob/master/lib/jqFhir.js
  2. When I took over this repo I assumed that jqFhir.js is just the build of fhir.js that gets built into the fhirclient (at least that should have been the initial idea). However, I have now discovered that jqFhir.js is in fact a modified version of the fhir.js build (example: https://github.com/smart-on-fhir/client-js/commit/08ac36bba0aff8d9b048b455e41c1b8e9a509dc4#diff-752a107efb4d573497b1eb9da29c01b5). By upgrading to the latest fhir.js and replacing the jqFhir file I have lost those "customizations".

This is nearly impossible to maintain! If I re-apply those customizations I will loose them again on the next fhir.js upgrade. That is why I would prefer to keep it as it is now, unless there is a good reason not to. It is not ideal, but at least it will be possible to upgrade fhir.js in the future. In other words, even though the change was not intentional, I think we should keep it if possible.

hansenms commented 5 years ago

It's fine with me either way. Do you want me to update my PR to fix all the apps with this call to match the new interface? I think I only did the medications one, since that is the one I was testing.

vlad-ignatov commented 5 years ago

Thank you for offering! I think I will accept #12 for now and lock the version until I feel confident that this fhirclient version can be considered stable. I want to give it some time and see if there are production issues.

Fixing the apps is the better long-term solution but we will have to update them all soon anyway, as part of the R4 support that we have planned.

P.S. Your PR is not enough to fix this because the second argument (refs at https://github.com/smart-on-fhir/sample-apps-stu3/pull/14/commits/568030cab5cdfa2860befd583976df4890ae0209#diff-16e48afff3396d51abf4b0f456f1adbcR56) is also changed. It used to be a function and it is an object now.