search

smart-on-fhir / growth-chart-app

Other
67 stars 98 forks source link

Wrong reference to fhir-client.js #53

Closed hansenms closed 5 years ago

hansenms commented 5 years ago

In this commit:

https://github.com/smart-on-fhir/growth-chart-app/commit/a5dc066cd1d4703016bcb5515dec983859fcffc2

the reference to the fhir client was changes from local (in node_modules) to an external reference, which is returning 404, specifically:

<script src="node_modules/fhirclient/fhir-client.js"></script>

was changed to:

<script src="https://combinatronics.com/smart-on-fhir/client-js/master/build/fhir-client.js"></script>

in index.html and launch.html

This is causing the app to break

@vlad-ignatov, I think this is a bug. Urgent fix needed.

vlad-ignatov commented 5 years ago

Should be fixed now.

Note that this is temporary. I've updated the app to use our latest client library, which is not officially released yet. Once it is, we will update our dependency and go back to that local reference.

Please let me know if you have any issues with the updated app using the new client.

hansenms commented 5 years ago

I can confirm that the current master is working.

I do have a follow-up question. I understand the need for changing things as you move to new versions, etc., but it would seem to make sense to do that in a branch rather than breaking master? Even if it is only temporary?

vlad-ignatov commented 5 years ago

I agree that such changes are typically done in a branch, but it is a little different here.

  1. It is a change that people need to see. Not something in a branch that nobody would know about until it's merged into master.
  2. It was "indirectly" broken (because a remote file is missing) only for a few hours on Sunday and you caught it! This wouldn't have happened if it wasn't on master.
  3. Master or not, the repository is live. I know you are frequently doing some kind of demos and I hope you are not automatically pulling the latest changes before running a demo. If you do, please consider using a fixed version or release. There is also a hosted version that you can use at https://examples.smarthealthit.org/growth-chart-app/. That is what we consider the production version and unlike GitHub, it should always be stable.

P.S. The hosted version will only work with the SMART sandbox. It can also be launched against an open FHIR server like so: https://examples.smarthealthit.org/growth-chart-app/launch.html?fhirServiceUrl=https://r2.smarthealthit.org&patientId=smart-7777702

hansenms commented 5 years ago

Hi @vlad-ignatov,

Thanks for the added context.

I am still not sure I fully understand why there is any utility (in terms of visibility or otherwise) in breaking the master branch, but if that is part of the development cycle, I will certainly make a note of it ;)

I do, in fact, pull the code in to demos. I have a demo sandbox at https://github.com/Microsoft/fhir-server-samples, where we pull the code into a web app. It is sort of part of the demo that we can pull it unmodified from the smart-on-fhir repo and have it work against our FHIR service, but that point could (of course) have been made with a fork and referencing a specific revision. I just thought it more illustrative to pull it straight from the master repo.

Do you guys tag the stable releases? If so that could be an option too.

vlad-ignatov commented 5 years ago

Oh yes! Once I finish this, I should definitely tag it. I guess we can keep this open until then and I'll let you know when its done.

vlad-ignatov commented 5 years ago

Done! Your tag is 0.1.4 - https://github.com/smart-on-fhir/growth-chart-app/releases/tag/0.1.4