Consider constraining allowed types of input to getReferenceString

[josh-]

Currently getReferenceString allows an input of Resource | Reference. However because all the properties on Reference are optional, this function allows any object to be passed in.

For example, the following TypeScript code compiles without error:

const string = getReferenceString({});
console.log('reference string is', string);

and prints the following:

reference string is undefined/undefined

this tripped us up in production recently.

---

would you consider potentially changing the signature of this function to enforce the string value being present on the passed-in input? for example something like so?

export function getReferenceString(
  input:
    | (Reference & { reference: string })
    | (Resource & { resourceType: ResourceType; id: string }),
): string {
  if (isReference(input)) {
    return input.reference;
  }
  return `${input.resourceType}/${input.id}`;
}

this would result in the above code sample no longer compiling successfully:

const string = getReferenceString({});

error TS2345: Argument of type '{}' is not assignable to parameter of type '(Reference<Resource> & { reference: string; }) | (Resource & { resourceType: "Appointment" | "AccessPolicy" | "Account" | "ActivityDefinition" | ... 157 more ... | "VisionPrescription"; id: string; })'.

[ThatOneBro]

I think this is a sane change to the signature, and would definitely prevent a lot of easy to prevent bugs. I'm all for it

What do you think @codyebberson ?

[codyebberson]

Sounds good in principle. Might be consider a breaking change, but agreed that it's for the better. We should try it on the medplum monorepo and see what breaks.

[codyebberson]

This causes quite a few build errors in our monorepo. They are all fixable, but I'm a little nervous that Medplum customers will need to make a ton of changes on next upgrade.

Maybe a v4 change?

Should getReferenceString() throw when there is not a valid reference string?

[josh-]

I think it makes sense for getReferenceString to throw when it will be producing an invalid string - and if this is done including in v4 sounds good to me.

[josh-]

Also thinking about this a bit more - we haven't yet enabled checkReferencesOnWrite (we are planning to), but I assume having that enabled would have caught this - wondering if you have any plans to enable this by default on new projects?

[codyebberson]

wondering if you have any plans to enable this by default on new projects?

At present, there is a somewhat severe performance penalty for enabling this feature, so we do not plan to enable by default.

[josh-]

No worries - makes sense 👍