Repository: FHIR/sushi
Pull request #386 (2020-04-23 20:15:17)

Load #dev packages from #current
Fixes https://github.com/FHIR/sushi/issues/374.

This makes it so now when a dependency is specified as `dev`, if it is not locally cached, it will be loaded from build.fhir.org in the same way as if it was specified as `current`.

I debated on this PR if I should call the newly loaded package that is being placed in the cache `x.y#dev` or `x.y#current`. In the end I chose `x.y#dev`, but there may be an argument for `x.y#current`. When the IG Publisher runs, if it can't find a package specified as `dev`, it loads the package from build.fhir.org, but it writes it to the cache as `x.y#current`. So in this way our approach would be inconsistent with the IG Publisher. But I think there is a reason to be inconsistent in this case. 

If we wrote the package to the cache as `x.y#current`, then each time SUSHI runs, it will not be able to find the `x.y#dev` package, so it will fall back to `current`, and reload the `x.y#current` package into the cache. The IG Publisher seems to avoid this scenario by not overwriting a `current` package every time. I assume the IG Publisher must somehow check to see if a `current` package has changed, and only overwrite it if there has been a change. But I'm not sure how the Publisher goes about this. SUSHI does not have smart checking like that, so it overwrites a `current` package every time.

Even if SUSHI had this smart checking, I still think it makes sense to write the package to the cache as `x.y#dev`. If the dependency is specified as `dev`, I would assume a user wouldn't want their cache to update even _if_ the current version on build.fhir.org changes, so the IG Publisher way of doing things confuses me a bit. To me it seemed to make the most sense to download the package one time and save it as `x.y#dev`, so now the user has a local copy that will not update unless they purposefully change their dependency version from `dev`. Hopefully this made it somewhat clear why I chose to write the package as `x.y#dev`, but if you think writing the package as `x.y#current` or some other approach makes more sense, suggest away!

ngfreiter: Hmm, not sure what is going on with GitHub actions here though.
cmoesel: So... I kind of feel like we should write it as `#current` in the cache.  I feel this way because:
* it is more consistent with the IG publisher
* the docs describe `#dev` as _"the latest local build of the IG"_, so I feel like anything marked `#dev` in the cache _should_ be a _local build_.

If someone has a dependency marked as `#dev`, but no local build, I think it's fair to log an info message indicating that `#dev` was not found and it will use `#current` instead.  Then at least they know why it's doing a fresh download.

I think it might also be worth taking 10-15 minutes to see if there is an easy way to determine if a build is current... Maybe it is easier than we think.
ngfreiter: Okey doke, I'll change to use `current` then.

I think if we are going to come up with a way to determine current builds, we should try to figure out what the IG Publisher does and do that, so we don't get into trouble at some point.
ngfreiter: What do you think makes sense to put in the `dependsOn` of the ImplementationGuide. I could see wanting to switch it to `#current` there as well, to indicate that because `#dev` is not present, we have given them the `#current` package. But I could also see leaving it as `#dev`, because that is the value they entered (presumably intentionally), and because the IG Publisher will see the `#dev`, look for `#dev` in the cache, not find it, and then fall back to `#current` just as we did. As a point of reference, when I have an IG Resource with `#dev` in the `dependsOn`, and I run it through the IG Publisher without having the package cached, the `#current` package is downloaded to the cache, but `#dev` still comes out the other end in the IG Resource. So my default would be to follow what the Publisher does and leave the dependency as `#dev` on the IG Resource, but call the package `#current` in the cache. But this time I'm asking before coding 😉 .
cmoesel: I agree with your assessment.  Keep it `#dev`.
ngfreiter: Ok so https://github.com/FHIR/sushi/pull/386/commits/9fd40d96971f118a587256a883e6f1fade65fcfa makes it so that it is written to the cache as `#current`.

I wasn't able to figure out how the IG Publisher handles updating `#current`. I think this could be a good task on its own to investigate further, but for now I wasn't able to figure out how to make the IG Publisher update the `#current` folder. I tried changing the dates of the package.json, the IG.json resource, and the ig.ini stored in the package folder, but none of that seemed to work to force an update. So as it stands SUSHI will just update `#current` every time.
ngfreiter: I've added it as a task to our backlog to investigate further: https://standardhealthrecord.atlassian.net/browse/CIMPL-364. Since at least as far as I can tell it isn't just a simple thing to add.

Patch:
diff --git a/src/errors/DevPackageLoadError.ts b/src/errors/DevPackageLoadError.ts
deleted file mode 100644
index 01adf88..0000000
--- a/src/errors/DevPackageLoadError.ts
+++ /dev/null
@@ -1,10 +0,0 @@
-import { Annotated } from './Annotated';
-
-export class DevPackageLoadError extends Error implements Annotated {
-  specReferences = ['https://confluence.hl7.org/display/FHIR/NPM+Package+Specification'];
-  constructor(public fullPackageName: string) {
-    super(
-      `The package ${fullPackageName} could not be loaded locally. Dev packages must be present in local cache`
-    );
-  }
-}
diff --git a/src/errors/index.ts b/src/errors/index.ts
index e12b65a..b428c98 100644
--- a/src/errors/index.ts
+++ b/src/errors/index.ts
@@ -29,7 +29,6 @@ export * from './InvalidSumOfSliceMinsError';
 export * from './InvalidMaxOfSliceError';
 export * from './ParentNotDefinedError';
 export * from './PackageLoadError';
-export * from './DevPackageLoadError';
 export * from './CurrentPackageLoadError';
 export * from './InvalidUriError';
 export * from './MissingSnapshotError';
diff --git a/src/fhirdefs/load.ts b/src/fhirdefs/load.ts
index da6fd9c..7d5dfcb 100644
--- a/src/fhirdefs/load.ts
+++ b/src/fhirdefs/load.ts
@@ -1,5 +1,5 @@
 import { FHIRDefinitions } from './FHIRDefinitions';
-import { PackageLoadError, DevPackageLoadError, CurrentPackageLoadError } from '../errors';
+import { PackageLoadError, CurrentPackageLoadError } from '../errors';
 import fs from 'fs-extra';
 import path from 'path';
 import os from 'os';
@@ -23,8 +23,8 @@ export async function loadDependency(
   FHIRDefs: FHIRDefinitions,
   cachePath: string = path.join(os.homedir(), '.fhir', 'packages')
 ): Promise<FHIRDefinitions> {
-  const fullPackageName = `${packageName}#${version}`;
-  const loadPath = path.join(cachePath, fullPackageName, 'package');
+  let fullPackageName = `${packageName}#${version}`;
+  let loadPath = path.join(cachePath, fullPackageName, 'package');
   let loadedPackage: string;
   if (version != 'current') {
     logger.info(`Checking local cache for ${fullPackageName}...`);
@@ -38,9 +38,16 @@ export async function loadDependency(
   if (!loadedPackage) {
     let packageUrl: string;
     if (version === 'dev') {
-      // Dev packages must be present in local FHIR cache
-      throw new DevPackageLoadError(fullPackageName);
-    } else if (version === 'current') {
+      // When a dev package is not present locally, fall back to using the current version
+      // as described here https://confluence.hl7.org/pages/viewpage.action?pageId=35718627#IGPublisherDocumentation-DependencyList
+      logger.info(
+        `Falling back to ${packageName}#current since ${fullPackageName} is not locally cached. To avoid this, add ${fullPackageName} to your local FHIR cache.`
+      );
+      version = 'current';
+      fullPackageName = `${packageName}#${version}`;
+      loadPath = path.join(cachePath, fullPackageName, 'package');
+    }
+    if (version === 'current') {
       // Current packages need to be loaded using build.fhir.org
       const baseUrl = 'http://build.fhir.org/ig';
       const res: { 'package-id': string; date: string; repo: string }[] = await rp.get({


Test patch:
diff --git a/test/fhirdefs/load.test.ts b/test/fhirdefs/load.test.ts
index 2a5fe38..221bb6c 100644
--- a/test/fhirdefs/load.test.ts
+++ b/test/fhirdefs/load.test.ts
@@ -154,6 +154,32 @@ describe('#loadDependency()', () => {
     expect(tarSpy.mock.calls[0][0].cwd).toBe(path.join('foo', 'hl7.fhir.us.core.r4#current'));
   });
 
+  it('should load the current package from build.fhir.org when a dev package is loaded and not locally cached', async () => {
+    await expect(loadDependency('hl7.fhir.us.core.r4', 'dev', defs, 'foo')).rejects.toThrow(
+      'The package hl7.fhir.us.core.r4#current could not be loaded locally or from the FHIR package registry'
+    );
+    expect(
+      loggerSpy
+        .getAllMessages('info')
+        .some(message =>
+          message.match(
+            /Falling back to hl7.fhir.us.core.r4#current since hl7.fhir.us.core.r4#dev is not locally cached./
+          )
+        )
+    ).toBeTruthy();
+    expect(requestSpy.mock.calls[0]).toEqual([
+      {
+        json: true,
+        uri: 'http://build.fhir.org/ig/qas.json'
+      }
+    ]);
+    expect(requestSpy.mock.calls[1][0].uri).toBe(
+      'http://build.fhir.org/ig/HL7/US-Core-R4/package.tgz'
+    );
+    expect(ensureDirSpy.mock.calls[0]).toEqual([path.join('foo', 'hl7.fhir.us.core.r4#current')]);
+    expect(tarSpy.mock.calls[0][0].cwd).toBe(path.join('foo', 'hl7.fhir.us.core.r4#current'));
+  });
+
   it('should throw CurrentPackageLoadError when a current package is not listed', async () => {
     await expect(loadDependency('hl7.fhir.us.core', 'current', defs, 'foo')).rejects.toThrow(
       'The package hl7.fhir.us.core#current is not available on http://build.fhir.org/ig/qas.json, so no current version can be loaded'
@@ -180,12 +206,6 @@ describe('#loadDependency()', () => {
       }
     ]);
   });
-
-  it('should throw DevPackageLoadError when a dev package version is not locally present', async () => {
-    await expect(loadDependency('test', 'dev', defs, 'somePath')).rejects.toThrow(
-      'The package test#dev could not be loaded locally. Dev packages must be present in local cache'
-    );
-  });
 });
 
 describe('#loadCustomResources', () => {