feat: track 'last online' time in localStorage

[KaiVandivier]

This feature can be used by apps or the header bar to display the 'last online' status. It's stored in localStorage so that the value can persist between sessions and be used between apps (although it will only be updated in apps that use PWA or use the most recent UI with the new header bar).

In the tests, I tried mocking the Date class and Date.now(), but both approaches either had issues with typescript or would make the test fail for mysterious reasons :(

[mediremi]

Should we return lastOnline in this hook?

// Only fetch if `online === false` as local storage is synchronous and disk-based
const lastOnline = !online ? localStorage.getItem(lastOnlineKey) : null

return {
  online,
  offline: !online,
  lastOnline: lastOnline && new Date(lastOnline)
}

Or have a separate useLastOnlineHook?

[KaiVandivier]

oh yeah lol 🤦‍♂️

[KaiVandivier]

I like adding it to this one 👍

[KaiVandivier]

Interesting note: the way this is implemented currently using useEffect to set the lastOnline value in localStorage, that suggested code doesn't work to return the most recent lastOnline value from the hook because there isn't a rerender once the value in localStorage is changed. I'll have to refactor a bit to couple the localStorage change with the online state change so that the hook returns the most up-to-date lastOnline value, ideally without another rerender

[mediremi]

What about using the global store instead of local storage?

[KaiVandivier]

Using the global store would be a useful part of 'Single online status provider' solution that Austin mentions below, but using just the global store alone wouldn't be able to track the status across sessions

[amcgee]

When useOnlineStatus is used potentially dozens of times across the app, this useEffect will run for each instance and update the localStorage at that time. This isn't practical and could maybe even cause race conditions / stale data / performance issues (localStorage is synchronous).

Instead, I would recommend setting this localStorage value only if the saved state is out of sync (i.e. if state is null when responding to type === 'offline', then and only then set the value to the last online time) in the debounced updateState callback. It should be read from localStorage in the render loop. (note there could possibly be performance implications from this on low-spec machines)

The best solution might be to implement a single global online/offline event handler which sets both the online status and the last online time in a global state provider (reading and writing to localStorage at that time) and which would then be consumed by the useOnlineStatus hook. This should reduce the possibility of race conditions. I realized that's a bit of a refactor though, might be something to look into in the future. It would also possibly allow for manually setting online/offline time from that central state handler in the future

[KaiVandivier]

Thanks for the feedback @amcgee!

What you describe in the second paragraph sounds like the direction I was going with a refactor I mentioned above, and I pushed a change that I think implements what you describe -- did I understand you correctly?

Unfortunately it seems like reading the value from local storage in the render loop doesn't work, according to the tests 🤔 can the localStorage implementation in Jest be trusted? Update: reordering setState after localStorage changes seems to work

I agree that a single handler/provider would be best and I think we should go for that in another iteration 👍

[amcgee]

minor: useMemo might help mitigate possible performance issues here - without it any time the parent rerenders it might create a new options object and cause an unnecessary localStorage lookup - so long as we can assume the value of localStorage.getItem(lastOnlineKey) will never change once offline is set.

Suggested change

	const lastOnline = !online && localStorage.getItem(lastOnlineKey)
	const lastOnline = useMemo(() => !online && localStorage.getItem(lastOnlineKey), [online])

[KaiVandivier]

So here's something I noticed: if you're offline and open and app, and lastOnline is not set, the hook doesn't set lastOnline initially. It's easy enough to set it in localStorage:

useEffect(() => {
    if (!online && !localStorage.getItem('lastOnline')) {
        localStorage.setItem(etc.)
    }
}, [])

but then I'm not sure what the most ideal way of updating the lastOnline value returned by the hook -- is there a way to do that without a re-render?

[amcgee]

Haven't tested but code looks good!

[amcgee]

@KaiVandivier but what would we even set lastOnline to in that case...?

[KaiVandivier]

I was thinking just new Date(), like it does when switching from online to offline... that may not even be necessary though; 'Offline' is probably all the information a user needs in the case that the app was never online

[dhis2-bot]

🎉 This PR is included in version 2.10.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀