feat: dhis2 connection status [LIBS-315] #1203
Conversation
There was a problem hiding this comment.
I think before you continue with this PR we need to settle on a valid strategy to check the connectivity status. I have proposed one in this Google Doc.
| /** | ||
| * Band-aid for the ping API, which can throw an error when being processed | ||
| * as JSON. Hopefully this will be superseded by a new ping endpoint: | ||
| * https://dhis2.atlassian.net/browse/DHIS2-14531 | ||
| */ | ||
| const getAcceptHeader = (query: ResolvedResourceQuery): string => { | ||
| if (query.resource === 'system/ping') { | ||
| return 'text/plain' | ||
| } | ||
|
|
||
| return 'application/json' | ||
| } |
There was a problem hiding this comment.
This can be removed if we get a dedicated ping endpoint before merging, as requested in the Jira issue mentioned in the comment: DHIS2-14531: Create dedicated ping endpoint
| <CustomDataProvider data={{}}> | ||
| <OfflineProvider | ||
| offlineInterface={mockOfflineInterface} | ||
| {...props} | ||
| > | ||
| <TestControls id={'1'} {...props} /> | ||
| <TestSection id={'1'} {...props} /> | ||
| </OfflineProvider> | ||
| </CustomDataProvider> |
There was a problem hiding this comment.
The OfflineProvider now uses a data provider to use the engine to make a data query to /api/system/ping
The same changes will be repeated in the next few test files
| // The offline interface persists the latest update from the SW so that | ||
| // this hook can initialize to an accurate value. The App Adapter in the | ||
| // platform waits for this value to be populated before rendering the | ||
| // the App Runtime provider (including this), but if that is not done, | ||
| // `latestIsConnected` may be `null` depending on the outcome of race | ||
| // conditions between the SW and the React component tree. | ||
| const [isConnected, setIsConnected] = useState( | ||
| offlineInterface.latestIsConnected | ||
| ) |
There was a problem hiding this comment.
Implemented in this commit: dhis2/app-platform@5644f0d
| - [Offline tools](advanced/offline) | ||
| - [Offline tools](advanced/offline/) | ||
| - [Cacheable Sections](advanced/offline/CacheableSections.md) | ||
| - [useDhis2ConnectionStatus](advanced/offline/useDhis2ConnectionStatus.md) | ||
| - [useOnlineStatus](advanced/offline/useOnlineStatus.md) |
There was a problem hiding this comment.
I improved the organization and discoverability here
docs/advanced/offline/README.md
Outdated
|
|
||
| The app platform provides some support for PWA features, including a `manifest.json` file for installability and service worker which can provide offline caching. In addition to those features, the app runtime provides support for ["cacheable sections"](advanced/offline/CacheableSections), which are sections of an app that can be individually cached on-demand. The [`useCacheableSection` hook](advanced/offline/CacheableSections#usecacheablesection-api) and the [`CacheableSection` component](advanced/offline/CacheableSections#cacheablesection-api) provide the controls for the section and the wrapper for the section, respectively. The [`useCachedSections` hook](advanced/offline/CacheableSections#usecachedsections-api) returns a list of sections that are stored in the cache and a function that can delete them. | ||
|
|
||
| An important tool for offline-capable apps is the [`useDhis2ConnectionStatus` hook](advanced/offline/useDhis2ConnectionStatus.md), which can be used to determine whether or not the app can connect to the DHIS2 server. There is also a [`useOnlineStatus` hook](advanced/offline/useOnlineStatus.md) which returns the whether or not the client is connected to the internet, but `useDhis2ConnectionStatus` is probably the one you want to use. |
There was a problem hiding this comment.
Consider adding an explanation as to why this is the one people would want to use. I.e. provide the most common example of local/on-site/intranet DHIS2 Core instances.
There was a problem hiding this comment.
Added a bit here: 1585c50
| headers: { | ||
| ...requestHeadersForContentType(contentType), | ||
| Accept: getAcceptHeader(query), | ||
| }, |
There was a problem hiding this comment.
Perhaps cleaner to call the getAcceptHeader helper in requestHeadersForContentType
There was a problem hiding this comment.
🤔 On second thought.... I can see why you kept this separate, since the helper is suffixed with ForContentType and the accept header does not actually depend on the content type. Still it would be in good company... requestBodyForContentType has committed the same sin.
On balance I don't think one option is significantly better than the other. And hopefully we can remove this helper soon anyway. So just leave it if you'd like.
| /** Called when SW reports updates from incidental network traffic */ | ||
| const onUpdate = useCallback( | ||
| ({ isConnected: newIsConnected }) => { | ||
| console.log('handling update from sw') |
There was a problem hiding this comment.
OK, but since this is a PR against master and I feel it's in its final stages we should now either get rid of them or keep them in but ensure they only show on the console in development mode. I.e. sth like this:
function devLog() {
if (process.env === 'development') {
console.log(arguments)
}
}
There was a problem hiding this comment.
I decided to approve this PR because the code is looking good. But before merging, please take care to either remove the console.log statements or replace them with something that only logs in development mode, see #1203 (comment).
There was a problem hiding this comment.
I've reviewed again, and only focussed on the commits that had been added approximately since the last time I reviewed, so starting at 1585c50 until the last commit. All looked good. to me. Great work @KaiVandivier
# [3.9.0](v3.8.0...v3.9.0) (2023-03-02) ### Features * dhis2 connection status [LIBS-315] ([#1203](#1203)) ([6a4156e](6a4156e))
|
🎉 This PR is included in version 3.9.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Part of LIBS-315
Adds a DHIS2 connection status service as part of the offline tools. Coordinates with the offline interface to deduce the status by incidental network requests, then sends pings periodically if there is no other traffic.
Depends on dhis2/app-platform#718; would also benefit from DHIS2-14531: Create dedicated ping endpoint
The spec
(This section is the same text as in the Platform PR above)
Notes for this PR
useOnlineStatushook which returns ConnectionStatus by default but could return NetworkStatus by passing anoptions.useNetworkStatusparameter, but that turned out to be complicated for several reasons (for example the rule of hooks). After discussing with Hendrik, we decided to leave the existing hook’s name, and export a newuseDhis2ConnectionStatushookuseOnlineStatushook has been renamed touseNetworkStatusto better reflect its function. Still, it’s exported asuseOnlineStatusfor backward-compatibilityuseDhis2ConnectionStatushook then does what it says on the tinTo test locally:
To do:
Done: