Skip to content
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

(feat) Create an application-wide context for shared state #976

Merged
merged 7 commits into from
May 13, 2024

Conversation

ibacher
Copy link
Member

@ibacher ibacher commented Apr 18, 2024

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. Ensure your PR title includes a conventional commit label (such as feat, fix, or chore, among others). See existing PR titles for inspiration.

For changes to apps

If applicable

  • My work includes tests or is validated by existing tests.
  • I have updated the esm-framework mock to reflect any API changes I have made.

Summary

This is currently a draft and more of a sketch of an idea than an actual implementation.

In essence, this actually ends up restoring something like the AppState that we used to have but did not use: a global state called the "context". The idea here is that contextually-important shared state can be pushed here and accessed from here. So, for example, the current session data could be stored and periodically refreshed or, for example, the current patient in a chart context (or current visit) could be stored here and hooks could defer to using it.

There are a few differences between this iteration and the AppState:

  1. All data is bound to a namespace, which effectively functions as a selector (with the ability to add sub-selectors)
  2. useAppContext() basically returns read-only copies of the data. The idea here is that one component defines and maintains the context and other components can leverage it, but not update it.
  3. useDefineAppContextNamespace and the OpenmrsAppContext React component will clear the managed namespace from the context when the managing component is removed from the vDOM.

The main usage of this from a client standpoint looks something like this, having one component that creates and provides the state and then any number of consumers.

In the provider, you might have something like:

const patient = usePatient();
useDefineAppContextNamespace<PatientContext>('patient', patient);

And then in the consumer, something like:

const { patientUuid } = useAppContext<PatientContext>(('patient');

Right now, this is pretty useless, but the goal is to enable some level of app-level caching beyond just relying on SWR de-duplicating requests.

Screenshots

Related Issue

Other

This setup may be too Redux-like to be a good idea in practice. While it doesn't explicitly require the use of reducers or state transitions, it does try to share a similar single point of responsibility for data updating, albeit in a much cruder way.

Copy link
Contributor

github-actions bot commented Apr 18, 2024

Size Change: -383 kB (-9.3%) ✅

Total Size: 3.73 MB

Filename Size Change
packages/shell/esm-app-shell/dist/openmrs.a92d61b4bf5165a8.js 0 B -382 kB (removed) 🏆
ℹ️ View Unchanged
Filename Size Change
packages/apps/esm-devtools-app/dist/344.js 27.3 kB 0 B
packages/apps/esm-devtools-app/dist/630.js 2.69 kB 0 B
packages/apps/esm-devtools-app/dist/667.js 6.96 kB 0 B
packages/apps/esm-devtools-app/dist/735.js 2.63 kB 0 B
packages/apps/esm-devtools-app/dist/788.js 42.9 kB 0 B
packages/apps/esm-devtools-app/dist/875.js 9.86 kB 0 B
packages/apps/esm-devtools-app/dist/884.js 15.2 kB 0 B
packages/apps/esm-devtools-app/dist/889.js 207 kB +696 B (+0.34%)
packages/apps/esm-devtools-app/dist/988.js 328 B 0 B
packages/apps/esm-devtools-app/dist/main.js 3.14 kB 0 B
packages/apps/esm-devtools-app/dist/openmrs-esm-devtools-app.js 3.19 kB 0 B
packages/apps/esm-implementer-tools-app/dist/15.js 62.2 kB 0 B
packages/apps/esm-implementer-tools-app/dist/271.js 716 B 0 B
packages/apps/esm-implementer-tools-app/dist/319.js 637 B 0 B
packages/apps/esm-implementer-tools-app/dist/426.js 24.8 kB 0 B
packages/apps/esm-implementer-tools-app/dist/460.js 735 B 0 B
packages/apps/esm-implementer-tools-app/dist/482.js 15.2 kB 0 B
packages/apps/esm-implementer-tools-app/dist/528.js 133 kB 0 B
packages/apps/esm-implementer-tools-app/dist/56.js 3.07 kB 0 B
packages/apps/esm-implementer-tools-app/dist/560.js 13.9 kB 0 B
packages/apps/esm-implementer-tools-app/dist/574.js 560 B 0 B
packages/apps/esm-implementer-tools-app/dist/587.js 2.92 kB 0 B
packages/apps/esm-implementer-tools-app/dist/620.js 126 kB 0 B
packages/apps/esm-implementer-tools-app/dist/625.js 562 B 0 B
packages/apps/esm-implementer-tools-app/dist/644.js 717 B 0 B
packages/apps/esm-implementer-tools-app/dist/657.js 7.01 kB 0 B
packages/apps/esm-implementer-tools-app/dist/71.js 6.97 kB 0 B
packages/apps/esm-implementer-tools-app/dist/735.js 2.63 kB 0 B
packages/apps/esm-implementer-tools-app/dist/757.js 560 B 0 B
packages/apps/esm-implementer-tools-app/dist/788.js 42.9 kB 0 B
packages/apps/esm-implementer-tools-app/dist/791.js 284 B 0 B
packages/apps/esm-implementer-tools-app/dist/807.js 559 B 0 B
packages/apps/esm-implementer-tools-app/dist/833.js 681 B 0 B
packages/apps/esm-implementer-tools-app/dist/86.js 6.71 kB 0 B
packages/apps/esm-implementer-tools-app/dist/889.js 207 kB +696 B (+0.34%)
packages/apps/esm-implementer-tools-app/dist/main.js 79 kB 0 B
packages/apps/esm-implementer-tools-app/dist/openmrs-esm-implementer-tools-app.js 3.31 kB 0 B
packages/apps/esm-login-app/dist/111.js 1.22 kB 0 B
packages/apps/esm-login-app/dist/126.js 2.5 kB 0 B
packages/apps/esm-login-app/dist/173.js 1.22 kB 0 B
packages/apps/esm-login-app/dist/224.js 256 B 0 B
packages/apps/esm-login-app/dist/236.js 272 B 0 B
packages/apps/esm-login-app/dist/240.js 364 B 0 B
packages/apps/esm-login-app/dist/271.js 718 B 0 B
packages/apps/esm-login-app/dist/272.js 264 B 0 B
packages/apps/esm-login-app/dist/319.js 679 B 0 B
packages/apps/esm-login-app/dist/336.js 234 B 0 B
packages/apps/esm-login-app/dist/363.js 30.5 kB 0 B
packages/apps/esm-login-app/dist/460.js 737 B 0 B
packages/apps/esm-login-app/dist/539.js 298 B 0 B
packages/apps/esm-login-app/dist/56.js 3.06 kB 0 B
packages/apps/esm-login-app/dist/574.js 577 B 0 B
packages/apps/esm-login-app/dist/625.js 579 B 0 B
packages/apps/esm-login-app/dist/627.js 257 B 0 B
packages/apps/esm-login-app/dist/63.js 16.5 kB 0 B
packages/apps/esm-login-app/dist/644.js 718 B 0 B
packages/apps/esm-login-app/dist/667.js 6.96 kB 0 B
packages/apps/esm-login-app/dist/673.js 284 B 0 B
packages/apps/esm-login-app/dist/735.js 2.63 kB 0 B
packages/apps/esm-login-app/dist/757.js 660 B 0 B
packages/apps/esm-login-app/dist/788.js 42.9 kB 0 B
packages/apps/esm-login-app/dist/807.js 897 B 0 B
packages/apps/esm-login-app/dist/833.js 684 B 0 B
packages/apps/esm-login-app/dist/836.js 22.5 kB 0 B
packages/apps/esm-login-app/dist/884.js 15.2 kB 0 B
packages/apps/esm-login-app/dist/889.js 207 kB +696 B (+0.34%)
packages/apps/esm-login-app/dist/main.js 57 kB 0 B
packages/apps/esm-login-app/dist/openmrs-esm-login-app.js 3.37 kB 0 B
packages/apps/esm-offline-tools-app/dist/271.js 1.18 kB 0 B
packages/apps/esm-offline-tools-app/dist/319.js 1.13 kB 0 B
packages/apps/esm-offline-tools-app/dist/460.js 1.29 kB 0 B
packages/apps/esm-offline-tools-app/dist/56.js 3.07 kB 0 B
packages/apps/esm-offline-tools-app/dist/574.js 1.04 kB 0 B
packages/apps/esm-offline-tools-app/dist/59.js 56.5 kB 0 B
packages/apps/esm-offline-tools-app/dist/625.js 1.04 kB 0 B
packages/apps/esm-offline-tools-app/dist/63.js 16.5 kB 0 B
packages/apps/esm-offline-tools-app/dist/644.js 1.18 kB 0 B
packages/apps/esm-offline-tools-app/dist/667.js 6.96 kB 0 B
packages/apps/esm-offline-tools-app/dist/735.js 2.63 kB 0 B
packages/apps/esm-offline-tools-app/dist/757.js 1.2 kB 0 B
packages/apps/esm-offline-tools-app/dist/788.js 42.9 kB 0 B
packages/apps/esm-offline-tools-app/dist/807.js 1.11 kB 0 B
packages/apps/esm-offline-tools-app/dist/833.js 1.21 kB 0 B
packages/apps/esm-offline-tools-app/dist/884.js 15.2 kB 0 B
packages/apps/esm-offline-tools-app/dist/889.js 207 kB +696 B (+0.34%)
packages/apps/esm-offline-tools-app/dist/922.js 90.9 kB 0 B
packages/apps/esm-offline-tools-app/dist/main.js 147 kB 0 B
packages/apps/esm-offline-tools-app/dist/openmrs-esm-offline-tools-app.js 3.29 kB 0 B
packages/apps/esm-primary-navigation-app/dist/271.js 267 B 0 B
packages/apps/esm-primary-navigation-app/dist/319.js 237 B 0 B
packages/apps/esm-primary-navigation-app/dist/460.js 264 B 0 B
packages/apps/esm-primary-navigation-app/dist/553.js 23.3 kB 0 B
packages/apps/esm-primary-navigation-app/dist/574.js 230 B 0 B
packages/apps/esm-primary-navigation-app/dist/625.js 232 B 0 B
packages/apps/esm-primary-navigation-app/dist/63.js 16.5 kB 0 B
packages/apps/esm-primary-navigation-app/dist/644.js 267 B 0 B
packages/apps/esm-primary-navigation-app/dist/667.js 6.97 kB 0 B
packages/apps/esm-primary-navigation-app/dist/735.js 2.64 kB 0 B
packages/apps/esm-primary-navigation-app/dist/757.js 238 B 0 B
packages/apps/esm-primary-navigation-app/dist/762.js 7.61 kB 0 B
packages/apps/esm-primary-navigation-app/dist/788.js 42.9 kB 0 B
packages/apps/esm-primary-navigation-app/dist/807.js 290 B 0 B
packages/apps/esm-primary-navigation-app/dist/833.js 257 B 0 B
packages/apps/esm-primary-navigation-app/dist/884.js 15.2 kB 0 B
packages/apps/esm-primary-navigation-app/dist/889.js 207 kB +697 B (+0.34%)
packages/apps/esm-primary-navigation-app/dist/958.js 22.7 kB 0 B
packages/apps/esm-primary-navigation-app/dist/main.js 47.6 kB 0 B
packages/apps/esm-primary-navigation-app/dist/openmrs-esm-primary-navigation-app.js 3.23 kB 0 B
packages/framework/esm-api/dist/openmrs-esm-api.js 16.3 kB +30 B (+0.18%)
packages/framework/esm-config/dist/openmrs-esm-module-config.js 8.02 kB 0 B
packages/framework/esm-context/dist/openmrs-esm-context.js 1.1 kB 0 B
packages/framework/esm-dynamic-loading/dist/openmrs-esm-dynamic-loading.js 2.75 kB 0 B
packages/framework/esm-error-handling/dist/openmrs-esm-error-handling.js 889 B 0 B
packages/framework/esm-extensions/dist/openmrs-esm-extensions.js 8.1 kB 0 B
packages/framework/esm-feature-flags/dist/openmrs-esm-feature-flags.js 1.67 kB 0 B
packages/framework/esm-framework/dist/126.openmrs-esm-framework.js 2.47 kB 0 B
packages/framework/esm-framework/dist/278.openmrs-esm-framework.js 14.5 kB 0 B
packages/framework/esm-framework/dist/530.openmrs-esm-framework.js 2.92 kB 0 B
packages/framework/esm-framework/dist/619.openmrs-esm-framework.js 6.49 kB 0 B
packages/framework/esm-framework/dist/645.openmrs-esm-framework.js 9.31 kB 0 B
packages/framework/esm-framework/dist/680.openmrs-esm-framework.js 6.13 kB 0 B
packages/framework/esm-framework/dist/735.openmrs-esm-framework.js 2.66 kB 0 B
packages/framework/esm-framework/dist/788.openmrs-esm-framework.js 42.9 kB 0 B
packages/framework/esm-framework/dist/openmrs-esm-framework.js 476 kB +1.84 kB (+0.39%)
packages/framework/esm-globals/dist/openmrs-esm-globals.js 770 B 0 B
packages/framework/esm-navigation/dist/openmrs-esm-navigation.js 9.35 kB 0 B
packages/framework/esm-offline/dist/openmrs-esm-offline.js 34.4 kB 0 B
packages/framework/esm-react-utils/dist/openmrs-esm-react-utils.js 15.6 kB +398 B (+2.62%)
packages/framework/esm-routes/dist/openmrs-esm-utils.js 1.46 kB 0 B
packages/framework/esm-state/dist/openmrs-esm-state.js 921 B +33 B (+3.72%)
packages/framework/esm-styleguide/dist/openmrs-esm-styleguide.js 103 kB +33 B (+0.03%)
packages/framework/esm-translations/dist/openmrs-esm-core-translations.js 1.59 kB 0 B
packages/framework/esm-utils/dist/openmrs-esm-utils.js 11.1 kB +37 B (+0.33%)
packages/shell/esm-app-shell/dist/11c63b65f96a8718.js 499 B 0 B
packages/shell/esm-app-shell/dist/1e0131662341578e.js 645 B 0 B
packages/shell/esm-app-shell/dist/2916d0aa7a9d5dc8.js 544 B 0 B
packages/shell/esm-app-shell/dist/2e49bd21ea50fd0d.js 1.58 kB 0 B
packages/shell/esm-app-shell/dist/3f6edf0e8b58a308.js 6.87 kB 0 B
packages/shell/esm-app-shell/dist/4a3e954c45d63305.js 645 B 0 B
packages/shell/esm-app-shell/dist/4a756bad65fec00c.js 0 B -6.87 kB (removed) 🏆
packages/shell/esm-app-shell/dist/56c2295bc732ae32.js 722 B 0 B
packages/shell/esm-app-shell/dist/651172ae1548469c.js 499 B 0 B
packages/shell/esm-app-shell/dist/98343ad4bb547c48.js 499 B 0 B
packages/shell/esm-app-shell/dist/b5151d35f680b40a.js 3.82 kB 0 B
packages/shell/esm-app-shell/dist/ba933133ad512cac.js 499 B 0 B
packages/shell/esm-app-shell/dist/fa89289c9d9645c9.js 519 B 0 B
packages/shell/esm-app-shell/dist/openmrs.839c3b78688ea76f.js 383 kB 0 B
packages/shell/esm-app-shell/dist/service-worker.js 45.9 kB -3 B (-0.01%)
packages/tooling/openmrs/dist/cli.js 2.88 kB 0 B
packages/tooling/openmrs/dist/commands/assemble.js 2.82 kB 0 B
packages/tooling/openmrs/dist/commands/build.js 1.34 kB 0 B
packages/tooling/openmrs/dist/commands/debug.js 545 B 0 B
packages/tooling/openmrs/dist/commands/develop.js 2.59 kB 0 B
packages/tooling/openmrs/dist/commands/index.js 438 B 0 B
packages/tooling/openmrs/dist/commands/start.js 851 B 0 B
packages/tooling/openmrs/dist/index.js 517 B 0 B
packages/tooling/openmrs/dist/runner.js 637 B 0 B
packages/tooling/openmrs/dist/utils/config.js 728 B 0 B
packages/tooling/openmrs/dist/utils/debugger.js 576 B 0 B
packages/tooling/openmrs/dist/utils/dependencies.js 648 B 0 B
packages/tooling/openmrs/dist/utils/helpers.js 395 B 0 B
packages/tooling/openmrs/dist/utils/importmap.js 3.07 kB 0 B
packages/tooling/openmrs/dist/utils/index.js 444 B 0 B
packages/tooling/openmrs/dist/utils/logger.js 368 B 0 B
packages/tooling/openmrs/dist/utils/npmConfig.js 830 B 0 B
packages/tooling/openmrs/dist/utils/untar.js 722 B 0 B
packages/tooling/openmrs/dist/utils/variables.js 192 B 0 B
packages/tooling/openmrs/dist/utils/webpack.js 278 B 0 B
packages/tooling/webpack-config/dist/index.js 3.61 kB 0 B

compressed-size-action

@brandones
Copy link
Collaborator

Cool idea, thanks for this. Right now it seems like we generally solve this kind of problem using hooks like usePatient and relying on the SWR cache. What problem is this addressing, or what improvement would this allow?

@ibacher
Copy link
Member Author

ibacher commented Apr 19, 2024

What problem is this addressing, or what improvement would this allow?

So it's aimed at addressing of few things:

  1. The shared SWR cache is not actually de-duplicating things as much as we would like. The Patient Chart on dev3 makes multiple requests to the /fhir2/R4/Patient/<uuid> endpoint (I count 11 on initial load). While it's true that SWR will ensure that the component is rendered with the first response and so the refetches don't necessarily mess with the rendering of the chart components, it's still got the potential to make the app feel slower, especially on, e.g., low-powered tablets. (Technically, this is SWR performing per-spec, since SWR really just means "returns the stale data, makes the request and then updates stale data with the new data"). Obviously cache invalidation is a very hard problem, but currently we don't even have an accessible cache to invalidate.
  2. I'd like to use this to replace a few of the stores that we're using, like the visit store, since that's both contextual data and the context in which the current visit is valid is a little narrower than a global store (a current visit makes the most sense in the scope of the patient chart, but less sense for most of the patient management, home page, and admin tools stuff). The React components added here are designed to enable temporarily setting values in the store while the parent component is active, giving us better control over the lifecycle of that contextual data. I think a similar structure is also useful for working with the "current encounter" concept we need to add to the chart.
  3. The real driver here is enabling context-specific dynamic rules, especially for things like extension rendering, so that we can support things like "only render the pregnancy dashboard if the patient is capable of being pregnant" via a config-driven approach (basically, optionally rendering extensions based on JS expressions). The idea here is to be able to have a feature similar to O2's require expressions.
    3b. Loading the context into a framework-agnostic store allows us to operate on the state outside of React, which is desirable for use-cases like 3, and also for reworking parts of the framework that operate outside React to interact with that state.

None of the 3 things I outlined there are specifically solved by this PR, but it's intended as a building block to solve them.

Concretely this would be things like:

  1. usePatient() checks the OpenmrsAppContext to see if there's a patient defined and if it's the correct patient (based on UUID). If it is, we just pull the patient from the OpenmrsAppContext; if it isn't, we can pull in the correct patient and update the state.
  2. useVisit() can check the "current visit" part of the OpenmrsAppContext and return the value there if its set; if it isn't we can fallback to the default logic.
  3. We can stop extensions from being available altogether if certain requirements aren't met rather than just making them render nothing.

@brandones
Copy link
Collaborator

Love it. Those are great reasons. This PR seems like a good way to start; I look forward to seeing how this evolves.

As a side note, when I was first designing the extension system, I imagined that extensions would receive props based on the context. So patientUuid, visitUuid, etc. would be passed automatically as props, and extensions would even make explicit what kind of context they required, so they could only be attached in a valid context. Maybe that's still "not gonna happen," but this at least lays the technical foundation for it.

The shared SWR cache is not actually de-duplicating things as much as we would like. The Patient Chart on dev3 makes multiple requests to the /fhir2/R4/Patient/ endpoint (I count 11 on initial load).

!!! Wow I had no idea, that's quite bad

@ibacher ibacher marked this pull request as ready for review May 9, 2024 19:13
@ibacher
Copy link
Member Author

ibacher commented May 9, 2024

Alright, I'm now happy this works as intended, albeit it's pretty minimally functional. The only current difference between this and a global store is that the value should only ever be a read-only copy of the value.

Copy link
Member

@denniskigen denniskigen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, @ibacher! Thanks.

@ibacher ibacher merged commit 9693a2b into main May 13, 2024
10 checks passed
@ibacher ibacher deleted the feat/openmrs-context branch May 13, 2024 14:25
@NethmiRodrigo NethmiRodrigo self-assigned this May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants