fix: handle low cli-app-scripts version #1349
Merged
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
KaiVandivier
commented
Aug 17, 2023
Comment on lines
-186
to
+204
| const contextValue = useMemo( | ||
| () => ({ | ||
| // in the unlikely circumstance that offlineInterface.latestIsConnected | ||
| // is `null` when this initializes, fail safe by defaulting to | ||
| // `isConnected: false` | ||
| isConnected: Boolean(isConnected), | ||
| isDisconnected: !isConnected, | ||
| lastConnected: isConnected | ||
| const contextValue = useMemo(() => { | ||
| // in the unlikely circumstance that offlineInterface.latestIsConnected | ||
| // is `null` or `undefined` when this initializes, fail safe by defaulting to | ||
| // `isConnected: true`. A ping or SW update should update the status shortly. | ||
| const validatedIsConnected = isConnected ?? true | ||
| return { | ||
| isConnected: validatedIsConnected, | ||
| isDisconnected: !validatedIsConnected, | ||
| lastConnected: validatedIsConnected |
There was a problem hiding this comment.
This is a small fix: the correct fail-safe should be to default to isConnected: true so the app can be accessed (the UI might be blocked or changed if isConnected === false)
KaiVandivier
commented
Aug 17, 2023
Comment on lines
+177
to
+186
| if (!offlineInterface.subscribeToDhis2ConnectionStatus) { | ||
| // Missing this functionality from the offline interface -- | ||
| // use a ping on startup to get the status | ||
| smartIntervalRef.current?.invokeCallbackImmediately() | ||
| console.warn( | ||
| 'Please upgrade to @dhis2/cli-app-scripts@>10.3.8 for full connection status features' | ||
| ) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
This bit protects against the compatibility error
KaiVandivier
commented
Aug 17, 2023
Comment on lines
-158
to
+160
| expect(result.current.isConnected).toBe(false) | ||
| expect(result.current.isDisconnected).toBe(true) | ||
| expect(result.current.lastConnected).toEqual(testCurrentDate) | ||
| expect(result.current.isConnected).toBe(true) | ||
| expect(result.current.isDisconnected).toBe(false) | ||
| expect(result.current.lastConnected).toBe(null) |
There was a problem hiding this comment.
See the fix to the fail-safe below
kabaros
approved these changes
Dec 14, 2023
kabaros
approved these changes
Dec 14, 2023
dhis2-bot
added a commit
that referenced
this pull request
Dec 14, 2023
## [3.10.1](v3.10.0...v3.10.1) (2023-12-14) ### Bug Fixes * handle low cli-app-scripts version [LIBS-501] ([#1349](#1349)) ([d15bce1](d15bce1))
|
馃帀 This PR is included in version 3.10.1 馃帀 The release is available on:
Your semantic-release bot 馃摝馃殌 |
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.
Implements LIBS-501
Description
With the connection status changes in 3.9.0, I inadvertently created a dependency in PWA apps on
@dhis2/cli-app-scripts> 10.3.0 -- if an app fits these kinda specific circumstances, it will get an error:@dhis2/app-runtime>= 3.9.0@dhis2/cli-app-scriptsbetween 10.0.0 and 10.3.0,The circumstance is pretty narrow... but it could happen to the Data Entry app right now if it upgraded
@dhis2/app-runtimeto take advantage of the daylight savings timezone fix 馃槵The changes in this PR handle these missing
cli-app-scriptsfeatures in versions below 10.3.0 to avoid a breaking change inapp-runtimefor PWA apps (apps not using PWA won't use the connection status):offlineInterface.latestIsConnectedofflineInterface.subscribeToDhis2ConnectionStatusThere's also a small (but I think important) fix for fail-safe logic to prevent app UI from being blocked in case something else fails
Known issues
I believe these two drawbacks are both unlikely and non-breaking which makes them non-ideal but tolerable. The console warning advises upgrading cli-app-scripts versions to improve on those two points
Testing
Here's how I tested this:
app-platformrepo, I checked out thev10.2.3tag (and ranyarnI think)app-runtime, I built the@dhis2/app-service-offlinepackagebuilddir into thenode_modulesin the root of theapp-platformdir (<app-runtime>/services/offline/build=><app-platform>/node_modules/@dhis2/app-service-offline/build)examples/pwa-app/.d2/shell/node_modules/.cachedirapp-platformto verify that it doesn't crash (yarn workspace pwa-app start)Screenshots
Before:
The PWA app in the
app-platformrepo @ v10.2.3 tag, with@dhis2/app-runtimeat3.9.0in theapp-shell:After:
Same conditions, after building
app-service-offlinefrom@dhis2/app-runtimefrom this branch and manually adding it to thenode_modulesin theapp-platformrepo: