-
Notifications
You must be signed in to change notification settings - Fork 214
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) O3-4277: Enable app-specific SWRConfig #1253
base: main
Are you sure you want to change the base?
(feat) O3-4277: Enable app-specific SWRConfig #1253
Conversation
CC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Twiineenock . I think we should be able to have app-specific SWRConfig without changes to core. I'm thinking of just wrapping the root component in root.comonent.tsx
(in the ward app and the service queues app) with <SWRConfig>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I support this change. Doing it this way has two advantages:
- We can merge custom SWR config options with our application default options
- We avoid creating extra nested SWRConfig contexts
Maybe @ibacher or @denniskigen can weigh in, since @chibongho and I are at odds here?
@@ -31,6 +31,7 @@ export interface ComponentDecoratorOptions { | |||
featureName: string; | |||
disableTranslations?: boolean; | |||
strictMode?: boolean; | |||
swrConfig?: Partial<typeof defaultSwrConfig>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should really be Partial<Omit<SWRConfig, 'fetcher'>>
, I think, or something similar.
const opts = Object.assign({}, defaultOpts, userOpts); | ||
const opts = { ...defaultOpts, ...userOpts }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this non-functional change?
My own vote here is that we merge this or something like it in. I don't see any downsides to it and it does make it possible for app-specific default configurations to be applied in a relatively low-effort way. I don't think there's any reason that we can't just support both this and adding a nested SWRConfig, if someone feels like doing so. |
2c2970f
to
295e06e
Compare
Requirements
feat
,fix
, orchore
, among others). See existing PR titles for inspiration.For changes to apps
If applicable
Summary
Description
Work in this PR allows app-specific SWR configurations for example the service queues and ward apps, ensuring their data refresh rates align with their real-time requirements.
Background
Both the service queues and ward apps are used to display near real-time dashboards for OPD/IPD workflows. A recent global configuration change in the SWR library increased the default data cache time to 30 minutes. While this change optimizes performance for many apps, it is not suitable for apps requiring more frequent updates.
Screenshots
Related Issue
https://openmrs.atlassian.net/browse/O3-4277
Other