-
Notifications
You must be signed in to change notification settings - Fork 13
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
fix: handle low cli-app-scripts version #1349
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bit protects against the compatibility error
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the fix to the fail-safe below
## [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 📦🚀 |
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-scripts
between 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-runtime
to take advantage of the daylight savings timezone fix 😬The changes in this PR handle these missing
cli-app-scripts
features in versions below 10.3.0 to avoid a breaking change inapp-runtime
for PWA apps (apps not using PWA won't use the connection status):offlineInterface.latestIsConnected
offlineInterface.subscribeToDhis2ConnectionStatus
There'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-platform
repo, I checked out thev10.2.3
tag (and ranyarn
I think)app-runtime
, I built the@dhis2/app-service-offline
packagebuild
dir into thenode_modules
in the root of theapp-platform
dir (<app-runtime>/services/offline/build
=><app-platform>/node_modules/@dhis2/app-service-offline/build
)examples/pwa-app/.d2/shell/node_modules/.cache
dirapp-platform
to verify that it doesn't crash (yarn workspace pwa-app start
)Screenshots
Before:
The PWA app in the
app-platform
repo @ v10.2.3 tag, with@dhis2/app-runtime
at3.9.0
in theapp-shell
:After:
Same conditions, after building
app-service-offline
from@dhis2/app-runtime
from this branch and manually adding it to thenode_modules
in theapp-platform
repo: