-
Notifications
You must be signed in to change notification settings - Fork 12
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
base: master
Are you sure you want to change the base?
Changes from all commits
4bfe16f
b15a1ab
f0ec2ad
3327723
5672b60
f1fdd5f
e9252d3
29a2c6f
b7e4c16
af456a2
1e9dda2
cd99b54
292f88f
e55b82f
6b3e34a
cfa7893
bb4b556
d24d959
47c1be0
6cbd380
8bf4ac6
1b6dda6
b6fbb11
7d9993d
34a15ec
1e915a5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,96 @@ | ||
import { | ||
useCacheableSection, | ||
CacheableSection, | ||
useConfig, | ||
} from '@dhis2/app-runtime' | ||
import { CenteredContent, CircularLoader, CssVariables, Layer } from '@dhis2/ui' | ||
import PropTypes from 'prop-types' | ||
import React, { useEffect } from 'react' | ||
import { getPWAInstallationStatus } from '../../modules/getPWAInstallationStatus.js' | ||
|
||
const LoadingMask = () => { | ||
return ( | ||
<Layer> | ||
<CenteredContent> | ||
<CircularLoader /> | ||
</CenteredContent> | ||
</Layer> | ||
) | ||
} | ||
|
||
const CacheableSectionWrapper = ({ id, children, isParentCached }) => { | ||
const { startRecording, isCached, remove } = useCacheableSection(id) | ||
|
||
useEffect(() => { | ||
if (isParentCached && !isCached) { | ||
startRecording({ onError: console.error }) | ||
} else if (!isParentCached && isCached) { | ||
// Synchronize cache state on load or prop update | ||
// -- a back-up to imperative `removeCachedData` | ||
remove() | ||
} | ||
}, [isCached, isParentCached, remove, startRecording]) | ||
|
||
return ( | ||
<CacheableSection id={id} loadingMask={<LoadingMask />}> | ||
{children} | ||
</CacheableSection> | ||
) | ||
} | ||
|
||
CacheableSectionWrapper.propTypes = { | ||
children: PropTypes.node, | ||
id: PropTypes.string, | ||
isParentCached: PropTypes.bool, | ||
} | ||
|
||
export const DashboardPluginWrapper = ({ | ||
onInstallationStatusChange, | ||
children, | ||
cacheId, | ||
isParentCached, | ||
...props | ||
}) => { | ||
const { pwaEnabled } = useConfig() | ||
|
||
useEffect(() => { | ||
// Get & send PWA installation status now | ||
getPWAInstallationStatus({ | ||
onStateChange: onInstallationStatusChange, | ||
}).then(onInstallationStatusChange) | ||
}, [onInstallationStatusChange]) | ||
|
||
return props ? ( | ||
<div | ||
style={{ | ||
display: 'flex', | ||
height: '100%', | ||
overflow: 'hidden', | ||
}} | ||
> | ||
{pwaEnabled ? ( | ||
<CacheableSectionWrapper | ||
id={cacheId} | ||
isParentCached={isParentCached} | ||
> | ||
{children(props)} | ||
</CacheableSectionWrapper> | ||
) : ( | ||
children(props) | ||
)} | ||
<CssVariables colors spacers elevations /> | ||
</div> | ||
) : null | ||
} | ||
|
||
DashboardPluginWrapper.defaultProps = { | ||
isParentCached: false, | ||
onInstallationStatusChange: Function.prototype, | ||
} | ||
|
||
DashboardPluginWrapper.propTypes = { | ||
cacheId: PropTypes.string, | ||
children: PropTypes.func, | ||
isParentCached: PropTypes.bool, | ||
onInstallationStatusChange: PropTypes.func, | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
export const INSTALLATION_STATES = { | ||
READY: 'READY', | ||
INSTALLING: 'INSTALLING', | ||
} | ||
|
||
function handleInstallingWorker({ installingWorker, onStateChange }) { | ||
installingWorker.onstatechange = () => { | ||
if (installingWorker.state === 'activated') { | ||
// ... and update state to 'ready' | ||
onStateChange(INSTALLATION_STATES.READY) | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* Gets the current installation state of the PWA features, which is intended | ||
* to be reported from this plugin to the parent app to indicate that the | ||
* static assets are cached and ready to be accessed locally instead of over | ||
* the network. | ||
* | ||
* Returns either READY, INSTALLING, or `null` for not installed/won't install | ||
*/ | ||
export async function getPWAInstallationStatus({ onStateChange }) { | ||
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 | ||
} | ||
Comment on lines
+24
to
+45
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @KaiVandivier do you have any reply to these comments? |
||
|
||
// It shouldn't normally be possible to get here, but just in case, | ||
// listen for installations | ||
registration.onupdatefound = () => { | ||
// update state for this plugin to 'installing' | ||
onStateChange(INSTALLATION_STATES.INSTALLING) | ||
|
||
// also listen for the installing worker to become active | ||
const installingWorker = registration.installing | ||
if (!installingWorker) { | ||
return | ||
} | ||
handleInstallingWorker({ installingWorker, onStateChange }) | ||
} | ||
|
||
// and in the mean time, return null to show 'not installed' | ||
return null | ||
} |
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 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 bygetPWAInstallationStatus
.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 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:
await
ed if you want to control the flow when rendering the plugin for the first timeYour 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 anyreturn <state>