Use new FPL API #277
Use new FPL API #277
Conversation
The FPL version 2 package includes API changes. Update classes, functions, and tests that include loaded dependencies to use the new API. Most of this is acquired through SUSHI's FHIRDefinitions class, which extends FPL's BasePackageLoader class. The main thing to watch out for is that FHIRDefinitions must be initialized before use, which is an async operation. The LakeOfFHIR class is updated to have an instance of FHIRDefinitions for access to its fishing methods. Therefore, it also needs to be initialized asynchronously. There may be some possible redesigns of the LakeOfFHIR class in order to more fully make use of the new FPL API.
There was a problem hiding this comment.
This looks good, and thankfully doesn't seem to have affected too many things. I've left a few comments for your consideration. I've also noticed that when I run the tests, I'm now getting a lot of log messages about loading a virtual package. I think the spy used to suppress those but it seems it is not anymore. Have you looked into it?
package.json
Outdated
| @@ -73,10 +73,10 @@ | |||
| "diff": "^7.0.0", | |||
| "diff2html": "^3.4.48", | |||
| "fhir": "^4.12.0", | |||
| "fhir-package-loader": "^1.0.0", | |||
| "fhir-package-loader": "^2.0.0-beta.3", | |||
package.json
Outdated
| "flat": "^5.0.2", | ||
| "fs-extra": "^11.2.0", | ||
| "fsh-sushi": "^3.12.1", | ||
| "fsh-sushi": "file:../fsh-sushi-new-fpl.tgz", |
| constructor(public docs: WildFHIR[]) {} | ||
| readonly defs: FHIRDefinitions; | ||
|
|
||
| constructor(public docs: WildFHIR[]) { |
There was a problem hiding this comment.
If we don't expect nor want docs to be modified, we should probably make it private to ensure that doesn't happen by mistake.
There was a problem hiding this comment.
Ah. I see now that we need it for restockLake in the tests, so I guess it probably needs to stay as-is.
src/utils/MasterFisher.ts
Outdated
| public lakeOfFHIR = new LakeOfFHIR([]), | ||
| public external = new FHIRDefinitions() | ||
| public lakeOfFHIR: LakeOfFHIR, | ||
| public external: FHIRDefinitions |
There was a problem hiding this comment.
I noticed that you guard external below (e.g., this.external?.fishForFHIR(item, ...types)) -- so you should probably mark it optional here (e.g., public external?: FHIRDefinitions).
src/utils/Processing.ts
Outdated
| // Assign any missing ids where we can before filtering out duplicates so that all | ||
| // the definitions with the same resourceType without an id don't get filtered out. | ||
| lake.assignMissingIds(); | ||
| lake.removeDuplicateDefinitions(); |
There was a problem hiding this comment.
Is this something we always want to do? If so, should we just make it a part of LakeOfFHIR.prepareDefs() (and then make these functions private)?
| await restockLake( | ||
| lake, | ||
| path.join(__dirname, 'fixtures', 'simple-codesystem.json'), | ||
| path.join(__dirname, 'fixtures', 'unsupported-codesystem.json'), | ||
| path.join(__dirname, 'fixtures', 'simple-valueset.json'), | ||
| path.join(__dirname, 'fixtures', 'unsupported-valueset.json') | ||
| ); | ||
| fisher = new MasterFisher(lake, defs); |
There was a problem hiding this comment.
I expect that I am missing something, but... why do we have to constantly restock the lake in every test? Why don't we stock it in beforeAll like we used to? I don't see any modifications to the resources in the lake -- but maybe I'm missing it.
If this does indeed need to happen in every test, I think you should extract it into a function like getRestockedFisher (or something like that) since it looks like this logic is repeated a lot across the tests.
There was a problem hiding this comment.
The set of resources is slightly different in some of the tests, which is why I pulled it out of beforeAll. I'll make a helper function like you suggested.
test/utils/Processing.test.ts
Outdated
| @@ -743,9 +756,10 @@ describe('Processing', () => { | |||
|
|
|||
| it('should load FHIR R6 prerelease if specified', () => { | |||
| const defs = new FHIRDefinitions(); | |||
| // jest.spyOn(defs, 'loadPackage').mockImplementation(replacementLoadPackage); | |||
There was a problem hiding this comment.
Can this commented code be deleted?
For LakeOfFHIR, since assignMissingIds and removeDuplicateDefinitions are preparatory steps, add them to the beginning of prepareDefs. Since they should only be used as these preparatory steps, the methods are also marked as private. FSH grammar supports designation on a concept in a CodeSystem. These test fixtures are therefore now supported and renamed accordingly. This means that since only a missing name and id makes a CodeSystem unprocessable, and an id will be added by LakeOfFHIR if it is missing, it means that any CodeSystem resource is processable after LakeOfFHIR.prepareDefs is called. Add helper function to simplify test setup in ResolveValueSetComponentRuleURLsOptimizer. Mark FHIRProcessor tests as async, since the calls to restokeLake should be awaited. Minor cleanup in some other files.
|
I've made updates based on review comments from @cmoesel , and since the published SUSHI and FPL dependencies are now available, I've marked this PR as ready for review. |
Description:
The FPL version 2 package includes API changes. Update classes, functions, and tests that include loaded dependencies to use the new API. Most of this is acquired through SUSHI's FHIRDefinitions class, which extends FPL's BasePackageLoader class. The main thing to watch out for is that FHIRDefinitions must be initialized before use, which is an async operation.
The LakeOfFHIR class is updated to have an instance of FHIRDefinitions for access to its fishing methods. Therefore, it also needs to be initialized asynchronously. There may be some possible redesigns of the LakeOfFHIR class in order to more fully make use of the new FPL API.
Testing Instructions:
This currently needs thenew-fplbranch of SUSHI as a dependency. Until this branch is part of a SUSHI release, you'll need to make that dependency yourself. Pull down thenew-fplbranch of SUSHI, build it,npm packit into a.tgzfile, then use that as the dependency. Until the dependency is available, this PR will be a draft.Update 2024/12/24: the updates to SUSHI and FPL are now available in published releases. The dependencies in this PR are updated accordingly.
There are a lot of changes to the tests due to the FPL API changes. There should be no changes in output FSH resulting from this change, so try it out with some FHIR resources and make sure the FSH you get is the same before and after the changes.
Related Issue:
N/A