(feat) O3-3759: Refactor usePatient hook to leverage useSWR #1156
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
denniskigen
changed the title
|
Size Change: -88.3 kB (-1.52%) Total Size: 5.72 MB
ℹ️ View Unchanged
|
denniskigen
commented
Sep 19, 2024
packages/framework/esm-react-utils/src/openmrsComponentDecorator.tsx
Outdated
Show resolved
Hide resolved
ibacher
reviewed
Sep 19, 2024
There was a problem hiding this comment.
Thanks @denniskigen! Some complications to this. Hope it's not too much trouble.
| @@ -1,10 +1,11 @@ | |||
| /** @module @category API */ | |||
| import useSWR from 'swr'; | |||
There was a problem hiding this comment.
I think, as a rule, we want to keep SWR and any React-specific stuff out of the esm-api library if possible.
There was a problem hiding this comment.
Should we use a different abstraction instead? Like useOpenmrsSwr?
There was a problem hiding this comment.
It's more a question of can we move the SWR-specific stuff just into the usePatient() hook in react-utils?
packages/framework/esm-api/src/shared-api-objects/current-patient.ts
Outdated
Show resolved
Hide resolved
denniskigen
force-pushed
the
feat/O3-3759
branch
from
2a2c1c6 to
9af15f8
Compare
denniskigen
force-pushed
the
feat/O3-3759
branch
from
9af15f8 to
7575569
Compare
denniskigen
force-pushed
the
feat/O3-3759
branch
from
effbe25 to
9e48ff5
Compare
denniskigen
force-pushed
the
feat/O3-3759
branch
from
9e48ff5 to
a2a25ed
Compare
denniskigen
force-pushed
the
feat/O3-3759
branch
2 times, most recently
from
272c0c0 to
2bbafef
Compare
denniskigen
force-pushed
the
feat/O3-3759
branch
from
2bbafef to
4336050
Compare
denniskigen
commented
Sep 19, 2024
Comment on lines
-1
to
-6
| import type { SyncItem } from '@openmrs/esm-framework/src/internal'; | ||
| import { | ||
| fetchCurrentPatient, | ||
| getFullSynchronizationItems, | ||
| getSynchronizationItems, | ||
| } from '@openmrs/esm-framework/src/internal'; |
There was a problem hiding this comment.
I've just reordered the imports.
Comment on lines
+1
to
+2
| import { useMemo } from 'react'; | ||
| import useSWR, { type SWRResponse } from 'swr'; |
denniskigen
changed the title
denniskigen
commented
Sep 19, 2024
Comment on lines
+3
to
+4
| import { fhirBaseUrl, openmrsFetch, type FetchConfig } from '../openmrs-fetch'; | ||
| import { type FetchResponse } from '../types'; |
denniskigen
changed the title
denniskigen
changed the title
ibacher
approved these changes
Sep 20, 2024
chibongho
reviewed
Sep 20, 2024
| data: patient, | ||
| error, | ||
| isValidating, | ||
| } = useSWR<NullablePatient, Error | null>(currentPatientUuid ? ['patient', currentPatientUuid] : null, () => |
There was a problem hiding this comment.
Do we need the array of keys here? From the doc, I think all keys in the array should be passed in as params in the fetcher function.
Should it be this instead?
useSWR(currentPatientUuid, (patientUuid) => fetchCurrentPatient(patientUuid!, {})
There was a problem hiding this comment.
From my testing, this doesn't work as well. More specifically, the deduplication aspect doesn't work at all. With the array key, we have more control over what constitutes a unique request. Per the docs, the cache key will be associated with the entire key argument. In my implementation, the url and token are both tied to the cache key. I think that's why the deduplication works.
denniskigen
added a commit
that referenced
this pull request
Sep 20, 2024
* (feat) useCurrentPatient should leverage useSWR * Commit API docs * Fixup * Increase framework test timeout to 20s * More fixes * Fix type annotations
|
I like this improvement @denniskigen @ibacher |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requirements
feat,fix, orchore, among others). See existing PR titles for inspiration.For changes to apps
If applicable
Summary
This PR refactors the
usePatienthook to leverage theuseSWRhook.useSWRprovides caching, deduping, and revalidation out of the box. Presently, multiple requests are made to theFHIRPatientresource when the user loads a patient chart (around 8 or so requests). By leveraging the deduplication capabilities of useSWR, we can whittle this down to a single request as demonstrated in the videos below.Screenshots
Current behaviour
Note how there's about 8 requests for the FHIR patient object:
current.mp4
New behaviour
The 8 or so requests have been whittled down to exactly 1:
new.mp4
Related Issue
Other