-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add Browser support for FPL #52
Conversation
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.
This is looking good. Given how similar the registry code is, I think I agree with your general approach not to create another class.
I've left a few comments, questions, and suggestions for your consideration.
- Separate FHIRRegistryClientOptions - Pass in options to SQLJSPackageDB.initialize - Simplify map creation in BrowserBasedPackageCache
@cmoesel - I think 73ee511 addresses your comments so far. I answered one of your questions above, but happy to chat more about it. Two other questions I have that came up while I was working on that commit:
|
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.
Thanks for the updates! Regarding your last two questions:
- I just spent some time trying to figure out how to spy on the
initSqlJs
function and I also couldn't figure it out without doing some funky things in the production (and test code). I don't think it's worth jumping through hoops. - I don't have a strong opinion on this, but I understand your point -- and I think I agree. We probably don't need that skipped test at all, so feel free to delete it.
I commented the test out with a comment in f64fc81. |
48bda76
to
2ee37fe
Compare
I found one other issue when using this in FSH Online that I fixed in 2617104. That commit rejects any packages that encountered an error or an error status code (like if a package can't be found because you made a typo in the Configuration in FSH Online, just as a random example and not because I just did that...). This way the error is correctly caught and handled and other packages can be downloaded. |
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.
One minor typo, other than that, everything is in good order!
Co-authored-by: Mint Thompson <[email protected]>
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.
Looks good! Thank you, Julia. It's really neat to have this built into FPL now so that others can use it!
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.
hooray!!!
Description: This PR adds support for using FPL in a browser and using the browser's IndexedDB as a PackageCache.
The
BrowserBasedPackageCache
class is a new PackageCache implementation that can be used to store resources in the browser's IndexedDB. Because the IndexedDB API is asynchronous, theBrowserBasedPackageCache
does some extra work to ensure the synchronous methods of the PackageCache interface still work. There is an asyncinitialize
function that looks in the IndexedDB to check what packages are already loaded and saves that information to a local map, and thecachePackageTarball
function stores resources in IndexedDB and to the local map. The local map is then used for the rest of the methods, likeisPackageInCache
, as those are all synchronous.There were also a few other changes that were needed when using in a browser, some of which I'd like your opinion on if there's a better way around it:
https.get
vsaxios.get
: I could not figure out how to haveaxios.get
return something that could be unzipped in the browser. I suspect I must have missed something, but I ended up usinghttps.get
, which is what FSH Online currently uses. If anyone reviewing this has an idea of how to configure axios to get it to work with thetar-stream
library or anything else to successfully unzip and get the data without a filesystem, let me know.useHttps
option inFHIRRegistryClient
: When I needed to usehttps.get
instead ofaxios.get
, I had originally created a separate RegistryClient (aBrowserFHIRRegistryClient
), but all the code was the same except for the actual GET request. Instead, I ended up adding an option to the existing FHIRRegistryClient in an effort to duplicate as little code as possible. I'm not sure if this is the best approach, so let me know what you think.nock
for thehttps.get
calls: I could not figure out how to use Jest and spyOn and mocked implementations to successfully return responses that were like whathttps.get
returns. I ended up using the same approach that FSH Online used, which usednock
. That does add another dependency, and it feels like Jest should be able to do the same, so if you know of another way around usingnock
, let me know.initialize(initSql)
inSQLJSPackageDB
: It turns out that I needed to pass a configuration option intoinitSqlJs()
when initializing in the browser, so I needed to be able to initialize it in FSH Online and pass it in toSQLJSPackageDB
.Testing Instructions:
Make sure the tests reflect the expected functionality. I also pushed up a new-fpl branch on FSH Online that uses this branch. Feel free to try out using FSH Online and making sure you can download and load packages.
Related Issue:
Fixes #29