Proposal: Reusable components in O3

[ibacher (Ian Bacher)]

Continuing from this Slack thread.

As O3 has developed, we have run into some use-cases that need reusable components that can be shared. So far, our solution has been to stick such components into the styleguide or other shared library. However, this doesn’t completely solve the issue for a few reasons:

1. The styleguide components often don’t have access to the same configuration machinery extensions do, meaning that they need to take more of their configuration as props.
2. It doesn’t give implementers really clear guidance about how to build re-usable components.

As a result of this, we have a proliferation of various ways of doing this from relatively complicated setup proposals to a proliferation of various “generic” components.

The goal of this proposal is to give us a way of providing shared components that:

• Doesn’t privilege esm-core over approaches easily implementable by code using the framework.
• Doesn’t require adding complexity to the configuration system or the rather simple needs that the configuration system is designed for (e.g., choosing a specific concept to use for a widget, etc.)

The heart of this proposal is to allow an app to define an “extension” using an export from a different app and perform some configuration of it via the extension’s meta property. So, say I have two apps: @openmrs/esm-ward-app and @digi-uw/esm-digi-app and @openmrs/esm-ward-app has an export called wardView defined in it’s index.ts that defines a “view” of a patient in the ward app. Then, in @digi-uw/esm-digi-app, I can create a new extension in that app’s routes.json that looks like this:

{
   "name": "digi-default-ward-view",
   "component": "@openmrs/esm-ward-app#wardView",
   "meta": {
       "elements": ["obs-text-admission-reason"]
    }
},
{
  "name": "obs-text-admission-reason",
  "component": "@openmrs/esm-core-components#obsText",
  "meta": {
      "concept": "DIGI:Admission Reason"
   }
}

So we have here two custom extensions added that expand on two existing exports, the wardView export from @openmrs/esm-ward-app and the obsText export from the (purely speculative) @openmrs/esm-core-components, with the Ward View configured to display the customized obsText element.

@dkigen @chibongho1 @samuel34 @mseaton @mogoodrich @vasharma05

[ibacher (Ian Bacher)]

Doesn’t this introduce coupling between two microfrontends?

Yes, it does and we’d need to build out some support for that.

That said, we should still be limiting RefApp components from referring specifically to other apps unless, like the @openmrs/esm-core-components app I mentioned, we build out an app that’s intended to always be there.

Can we do this with extension from the styleguide?

No. For technical reasons, that’s still not going to be supported.

[ibacher (Ian Bacher)]

On Slack @chibongho1 offered a few use-cases, so I’ll go through how I see them working with this proposal:

• A link to the patient chart, which can be configured to a custom link (say, to the O2 patient chart instead of O3). This component appears in the Active Visit table, queues table, and appointments table. (See screenshot)

  

  I think this component by itself probably doesn’t need this, but the table itself might and in the configuration for the table, you might specify that it uses the “CustomizablePatientLink” component.

• An element to display patient identifier, configurbale to show/hide various patient identifier types.

  I don’t see why this wouldn’t be able to be done with the existing config system, really.

• An element configured to display obs value of a particular concept. We can configure it, for example, to display visit notes.

  This is more the kind of thing I had in mind.

• A more complex element configure to display coded obs values of a particular concept as Carbon tags. We can configure different values to have different tag colors. (See screenshot)

  

  This one, I think, needs further discussion. Is the idea to have an optional “Postpartum hemorrhage” tag element that renders under some condition or is this a widget that gets fed certain conditions + some kind of configuration that says “Postpartum hemorrhage is read”, “Pregnancy complications are green”.

[chibongho1 (Chi Bong Ho)]

Thanks @ibacher. I still have concerns about props typings with extensions in general, but I like the overall idea.

I don’t see why this wouldn’t be able to be done with the existing config system, really.

Yeah, that sounds good to me. Stick with config system for simpler components.

This one, I think, needs further discussion. Is the idea to have an optional “Postpartum hemorrhage” tag element that renders under some condition or is this a widget that gets fed certain conditions + some kind of configuration that says “Postpartum hemorrhage is read”, “Pregnancy complications are green”.

Kind of both? The configuration defines the concept to fetch / display observations for, AND that the config also defines the colors of specific coded obs values (“Postpartum hemorrhage is red”, “XXX is green”). To extend the example above:

{
   "name": "digi-default-ward-view",
   "component": "@openmrs/esm-ward-app#wardView",
   "meta": {
       "elements": ["obs-text-admission-reason", "obs-coded-obs-pregnancy-compications"]
    }
},
{
  "name": "obs-text-admission-reason",
  "component": "@openmrs/esm-core-components#obsText",
  "meta": {
      "concept": "DIGI:Admission Reason"
   }
},
{
  "name": "obs-coded-obs-pregnancy-compications",
  "component": "@openmrs/esm-core-components#obsText",
  "meta": {
      "concept": "DIGI:pregnancy-complication-answer",
      "colors": {
        "DIGI:Postpartum hemorrhage": "red",
        "DIGI:XXX": "green"
      }
   }
}

I imagine the component itself to only take in the config and a list of Obs values. The actual fetching of Obs data is done outside of the component itself (since that can be very domain specific; the visits table could fetch obs values very differently from the ward app.) I think this means having a way for digi-default-ward-view to “walk” the config to aggregate all concepts we need to fetch obs values for, then ideally make 1 single request to fetch obs of concepts "DIGI:Admission Reason", "DIGI:pregnancy-complication-answer" for all patients in the ward.

Related, should we have better support for types within ConfigSchema? I’m thinking of adding the following:

• CabonTagColors
• Icons
• Concept (We have the ConceptUuid type, but I think we need a type that offers support for Concept names)
• Extensions (this will be useful for arrays like ["obs-text-admission-reason", "obs-coded-obs-pregnancy-compications"])

[ibacher (Ian Bacher)]

@chibongho1

I still have concerns about props typings with extensions in general

I mean, I’m not sure how we can enforce strong types through something like an extension system. I’m not trying to suggest this as a way around places where it makes sense to have styleguide components per se, but I think styleguide components have some clear limitations that means we need something in addition to them. (The weakness here being that continuously adding options to the styleguide config schema is going to be a pain, but there are things we want to interact with the config system. We have some way of describing a config for an extension, but not for a component).

@chibongho1

Kind of both?

I think for something like that, there’s maybe room for a both / and since in addition to meta, you can define an extensionConfigSchema.

@chibongho1

I think this means having a way for digi-default-ward-view to “walk” the config to aggregate all concepts we need to fetch obs values for

Actually having a component walk a set of configs sounds potentially very messy to implement, especially because that seems to require some uniformity in the config schema. I think what we actually need here is a mechanism for the child elements to “tell” the wardView component what data they need.

A few possible techniques would be:

• Custom DOM events (this gets around the “each extension has a React root” barrier or else we could just leverage Suspense, since this is basically exactly what it’s for)
• Some sort of data- attribute on an element in the DOM (again the “React root” thing).

@chibongho1

Extensions (this will be useful for arrays like ["obs-text-admission-reason", "obs-coded-obs-pregnancy-compications"])

This is the sort of thing I don’t think belongs in config schemas.

Apart from that, I think that all makes sense. Especially if we can get away from needing to specify concepts by UUID everywhere.

[chibongho1 (Chi Bong Ho)]

I mean, I’m not sure how we can enforce strong types through something like an extension system. I’m not trying to suggest this as a way around places where it makes sense to have styleguide components per se, but I think styleguide components have some clear limitations that means we need something in addition to them. (The weakness here being that continuously adding options to the styleguide config schema is going to be a pain, but there are things we want to interact with the config system. We have some way of describing a config for an extension, but not for a component).

Yeah, in practice, what that has meant for me is “don’t use extensions unless I really need to”, which seems like the right mindset for this thing as well. We can have both this and more styleguide elements.

I think for something like that, there’s maybe room for a both / and since in addition to meta, you can define an extensionConfigSchema.

This is the sort of thing I don’t think belongs in config schemas.

Agreed. I was thinking more of typings for meta if we are going to use ConfigSchema for it as well.

So what are the next steps? The visit history table can use some of this.

[mogoodrich (Mark Goodrich)]

@ibacher

An element to display patient identifier, configurbale to show/hide various patient identifier types.

I don’t see why this wouldn’t be able to be done with the existing config system, really.

Not to be naive, but what is the current way to do this?

[ibacher (Ian Bacher)]

@chibongho1

So what are the next steps?

I think it’s this set of tickets, but let me know if we’re missing something.

@mogoodrich

Not to be naive, but what is the current way to do this?

Something like a configuration schema that takes an array of patient identifier type UUIDs, I think. It wouldn’t look exceptionally different using meta either. That’s just more about where the definition lives.

[mogoodrich (Mark Goodrich)]

Thanks @ibacher and @chibongho!

Just FYI I’ve read through this and will continue to, but generally defer to you two and @dkigen on this as you have much more experience with this, but I’ll add my 2 cents where applicable.

[mogoodrich (Mark Goodrich)]

@ibacher

Something like a configuration schema that takes an array of patient identifier type UUIDs, I think. It wouldn’t look exceptionally different using meta either. That’s just more about where the definition lives.

Right… sorry, maybe this is obvious at this point, but do we have a convention/place where common configurations like this would live (“default patient identifier types to display”)? Our configuration system, when I’ve used it, seemed very “app” centric. Does the style guide support configuration like this?

[chibongho1 (Chi Bong Ho)]

Right… sorry, maybe this is obvious at this point, but do we have a convention/place where common configurations like this would live (“default patient identifier types to display”)? Our configuration system, when I’ve used it, seemed very “app” centric. Does the style guide support configuration like this?

Yeah, they are in esm-styleguide. Styleguide components (which are meant to be included / used in other EMS) load the configs a bit differently, by hardcoding the config namespace.

[ibacher (Ian Bacher)]

@mogoodrich

Our configuration system, when I’ve used it, seemed very “app” centric. Does the style guide support configuration like this?

I do think this is generally the right thing to do. For the most part, those components should exist in apps. Anything truly global should probably just use system settings / global properties.

[mseaton (Mike Seaton)]

@ibacher

I think this component by itself probably doesn’t need this, but the table itself might and in the configuration for the table, you might specify that it uses the “CustomizablePatientLink” component

Are you saying this would or would not be similar to the obs-text-admission-reason above, where you would do something like this:

{
  "name": "digi-patient-link",
  "component": "@openmrs/esm-core-components#patientLink",
  "meta": {
      "url": "/coreapps/clinicianfacing/patient.page?patientId={{patient.uuid}}"
   }
}

@ibacher

• An element to display patient identifier, configurbale to show/hide various patient identifier types. I don’t see why this wouldn’t be able to be done with the existing config system, really.
• An element configured to display obs value of a particular concept. We can configure it, for example, to display visit notes. This is more the kind of thing I had in mind.

I’m not 100% clear on the difference. Are you making this distinction with the assumption that the patient identifier element is something that should apply globally across the application, and the obs element is one that should apply just in a specific component? If these are both, say, configurable columns in a patient table, where there is a component that can render based on configured metadata, they seem the same to me?

@ibacher

I do think this is generally the right thing to do. For the most part, those components should exist in apps. Anything truly global should probably just use system settings / global properties.

I definitely think we could pull more globally from system setting / global properties. In practice thus far though, I haven’t seen much evidence of this happening. It’s far more common that I see the same configurations that already exist on the backend get duplicated on the frontend. Would be great if we can find a way to make it easier for frontend developers to know what is available on the backend and to add to it as appropriate.

[ibacher (Ian Bacher)]

@mseaton

If these are both, say, configurable columns in a patient table, where there is a component that can render based on configured metadata, they seem the same to me?

Yeah, contextually, it might vary. For something that shows/hides various identifier types, I was imagining that’s more something that belongs in a specific app for displaying a patient, like the patient banner app. Maybe if it’s something slotting into a table, it makes sense to have a the single definition component. That said, I’m still not wholly comfortable with customizable tables. I don’t think we yet really have a good solution for orchestrating the backend calls and, in some cases, it may make better sense to actually leverage the reporting module to create datasets that we then render in the frontend.

For what I was thinking with obs: it actually makes sense to have some generic components to render obs values based on the obs. Text-based obs aren’t really the main thing I was thinking of, though. More like, e.g., a component that renders numeric obs similar to how we do in the vitals table or a component that renders obs that store dates or times.

I realize the categorization here is a bit fuzzy. I don’t have a good principle for exactly when one solution works better than another.

@mseaton

It’s far more common that I see the same configurations that already exist on the backend get duplicated on the frontend.

If you can point those out, we can work on getting rid of them. The goal of the configuration system wasn’t to do away with global properties and, preferentially, if it can be done with an existing global property, we should use it. It mostly comes down to a lack of reliable documentation for these kinds of thing.