-
Notifications
You must be signed in to change notification settings - Fork 4
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
Integrate FPL v2 #195
Integrate FPL v2 #195
Conversation
- Removes downloading and loading definitions into IndexedDB and getting resources from IndexedDB for FHIRDefinitions -- moved to FPL - Adds new FPL, new SUSHI, and new GoFSH Note: FSHHelper tests are still broken
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.
I'm so glad this is working! Thank you! I left two comments about possible places we could extract functions to make the code a little cleaner. Let me know what you think!
@cmoesel - 65adeb5 pulled out the code for loading the FHIRDefinitions into a separate function and calls that function for SUSHI and GoFSH. I did have to add a little bit of specialized logic because the configuration that the GoFSH One thing I just noticed is that there's an error when GoFSH is run before SUSHI. This is because GoFSH is setting up a LakeOfFhir: const lake = new processor.LakeOfFHIR(docs);
await lake.prepareDefs(); However, the FHIRDefinitions that LakeOfFHIR uses at that point is uninitialized, so it tries to initialize it, but the FHIRDefinitions is made with the default packageDB and there's no way to provide the needed override to use the |
@cmoesel - I think this is ready for review! 65adeb5 - pulls out the code for loading the FHIRDefinitions into a separate function and calls that function for SUSHI and GoFSH. I did have to add a little bit of specialized logic because the configuration that the GoFSH loadExternalDependencies uses is a different type than the configuration than the SUSHI loadExternalDependencies. Let me know what you think. If you don't like it, it's fine by me if you revert it. That commit also pulls out the code that had to set up temp variables just to determine the GoFSH configuration into a separate function, which kind of hides its weirdness. e0ace2a - initializes sql.js once for SUSHI and once for GoFSH so it is already properly initialized for any other places that use it. That means we don't need the 1c217a9 - filters out any provided dependency that doesn't have a version. That makes it so if anyone has a typo in the configuration modal (like |
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 looks good. I have one request. Let me know if it is non-trivial.
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 looks good! Thank you, @jafeltra. You've now proven that the FPL browser-based implementations work -- and you've unstuck us from that prior version of SUSHI. Thanks!
Note: This PR is in marked as a draft because theJan 7 update: this is ready for review now.FSHHelpers.test.jss
suite does not run yet but the general approach to integrating FPL v2 and new SUSHI and GoFSH is stable.Description:
This PR updates FSH Online to use the new FPL v2 and newer versions of SUSHI and GoFSH that use FPL v2 as well.
This PR cleans up a good amount of code that was moved into FPL. I also tried to use some newer SUSHI functions where possible, rather than having FSH Online write its own versions.
The way I had to create to make some temporary definitions for GoFSH is a little odd, but it was the only way I could figure out how to use a FHIRProcessor to
processConfig
to get all the dependencies from an IG resource, which we needed to properly initialize the BrowserBasedPackageCache.Note the following changes still need to be made to this branch before it can be merged:
FSHHelpers.test.js
file cannot load thesql-wasm.wasm
file correctly right now, so that needs to be figured out. TheassetsInclude
configuration field is what makes this work when running a development or production server, so I guess the testing equivalent needs to be figured out.package.json
once they're available and the versions need to be updated in the TopBar component.Jan 7 update: these have all been done.
Testing Instructions:
Ttry using this locally in your browser to make sure there isn't functionality that I've missed that needs to be added in FPL or here. You should be able to:
Related Issue:
N/A