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: implement shared dashboard plugin wrapper #1672

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

edoardo
Copy link
Member

@edoardo edoardo commented Jun 4, 2024

Relates to dhis2/data-visualizer-app#3082
Relates to dhis2/line-listing-app#396
Relates to dhis2/maps-app#3232


Key features

  1. implement a shared dashboard plugin wrapper component

Description

This wrapper adds support for dashboard offline caching.
Before the logic was replicated in the plugin component of each analytics app.
Now all apps can use this wrapper and only implement their specific logic related to the props required to be passed down to the plugin rendering component.


Known issues

The Storybook build command fails has been updated in a separate PR and works.

It used to run automatically after the build command, so also in Github, which I'm not sure is necessary.
It has been removed in the PR just to be able to build and copy the artifact on d2-ci so it can be used in app's PRs.

I think the issue is related to the upgrade of cli-app-scripts which introduces Webpack 5, while the Storybook setup is for Webpack 4.

I'm not sure an upgrade of Storybook belongs to this PR, but after this PR is merged, Storybook is broken.

@HendrikThePendric
Copy link
Contributor

Since I don't think we actually use the storybook build anywhere, I think it is fine to not include a fix for it in this PR. We should probably create a new JIRA ticket with a slightly larger scope: both building the storybook, and then also publishing it somewhere (perhaps a netlify deploy?)

Copy link
Contributor

@HendrikThePendric HendrikThePendric left a comment

Choose a reason for hiding this comment

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

I've left some comments and am not sure if we should perhaps not merge this PR until that bug in the useCacheableSection hook is solved. But will approve for logistical reasons.

Comment on lines +61 to +60
// Get & send PWA installation status now
getPWAInstallationStatus({
onStateChange: onInstallationStatusChange,
}).then(onInstallationStatusChange)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a bit confused about these lines. I don't know why we have to pass the same function both as a parameter/option and then also call it ourselves. at first glance it appears to me that the .then() part is redundant and should be done by getPWAInstallationStatus.

Copy link
Contributor

@KaiVandivier KaiVandivier Aug 8, 2024

Choose a reason for hiding this comment

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

I did it this way to communicate that you can definitely expect a value "now" (-ish; the service worker registration API is asynchronous), and you may get another one or two updates later -- maybe that didn't end up being obvious though

The current design also has two small benefits that I can see, though they're currently not taken advantage of:

  1. A different handler can be used for the initial state value received than for the subsequent updates
  2. This structure can be easily awaited if you want to control the flow when rendering the plugin for the first time

Your suggested refactor in another comment would certainly work though, and you guys are welcome to implement it if you prefer 👍 it should be pretty simple: adding onStateChange(<state>) before any return <state>

Comment on lines +24 to +45
if (!navigator.serviceWorker) {
// Nothing to do here
return null
}

const registration = await navigator.serviceWorker.getRegistration()
if (!registration) {
// This shouldn't happen since this is a PWA app, but return null
return null
}

if (registration.active) {
return INSTALLATION_STATES.READY
}
// note that 'registration.waiting' is skipped - it implies there's an active one
if (registration.installing) {
handleInstallingWorker({
installingWorker: registration.installing,
onStateChange,
})
return INSTALLATION_STATES.INSTALLING
}
Copy link
Contributor

Choose a reason for hiding this comment

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

See my previous comment about the code in the useEffect hook. I guess you could say that this parts computes the initial state and I think we can add some calls to onStateChange here to avoid having to call then later on.

Copy link
Member Author

Choose a reason for hiding this comment

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

@KaiVandivier do you have any reply to these comments?
I believe was you that wrote this part originally.

@edoardo edoardo force-pushed the feat/dashboard-plugin-wrapper branch from 7c8d2c8 to 87873cf Compare July 12, 2024 11:51
@edoardo edoardo force-pushed the feat/dashboard-plugin-wrapper branch from 87873cf to 1615bb4 Compare August 5, 2024 09:04
@edoardo edoardo force-pushed the feat/dashboard-plugin-wrapper branch from 1a5c9ef to 292f88f Compare August 9, 2024 09:10
@edoardo
Copy link
Member Author

edoardo commented Aug 16, 2024

Since I don't think we actually use the storybook build anywhere, I think it is fine to not include a fix for it in this PR. We should probably create a new JIRA ticket with a slightly larger scope: both building the storybook, and then also publishing it somewhere (perhaps a netlify deploy?)

I created a task for this so we (hopefully) don't forget.

This has been done and merged already, see PR #1724 .

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