google / android-fhir

The Android FHIR SDK is a set of Kotlin libraries for building offline-capable, mobile-first healthcare applications using the HL7® FHIR® standard on Android.
https://google.github.io/android-fhir/
Apache License 2.0
465 stars 245 forks source link

Loading workerContext using the Knowledge manager #2449

Closed nsabale7 closed 2 months ago

nsabale7 commented 3 months ago

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).

Fixes #2448

Description Clear and concise code change description.

Alternative(s) considered Have you considered any alternatives? And if so, why have you chosen the approach in this PR?

Type Choose one: (Bug fix | Feature | Documentation | Testing | Code health | Builds | Releases | Other)

Screenshots (if applicable)

Checklist

owais-vd commented 3 months ago

@nsabale7 just a question about the worker-context. I'm not sure why we created two different worker-context.

  1. https://github.com/google/android-fhir/pull/2449/files#diff-0abdac65d1f5c5a9700253625ed1b9d36a465bc0b5fec7e7e9cf26c5abc02779R36
  2. https://github.com/google/android-fhir/pull/2449/files#diff-63f547d1547f3d76757693a33c9f6ff6b911b03ab2aa4280843f9ddcddee3821R201

It shouldn't be one? and where the loadWorkerContext method is being called?

nsabale7 commented 3 months ago
  1. https://github.com/google/android-fhir/pull/2449/files#diff-0abdac65d1f5c5a9700253625ed1b9d36a465bc0b5fec7e7e9cf26c5abc02779R36 A. This defines a default simple worker-context object which does not load any packages.
  2. https://github.com/google/android-fhir/pull/2449/files#diff-63f547d1547f3d76757693a33c9f6ff6b911b03ab2aa4280843f9ddcddee3821R201 A. This worker-context loads provided npmPackages and returns the object.
jingtang10 commented 3 months ago

fyi left a comment in the issue: https://github.com/google/android-fhir/issues/2448#issuecomment-1979375776

i think we need to resolve this before we can merge this pr

nsabale7 commented 3 months ago

@joiskash @owais-vd @dubdabasoduba @jingtang10 I have added test using outbreak package. Please review this PR.