LF-5104 Create and configure custom service worker that can be extended to cache any number of endpoints#4005
Conversation
This is copied from the POC branch except I had added it to offlineDetector on that branch (where it doesn't go; I was just being quick)
I'm not sure I'll stick with it, but this was what was tested before
Pass status and throw generic Error to avoid importing from _private
…ervice-worker-that-can-be-extended-to-cache-any-number-of-endpoints
…ervice-worker-that-can-be-extended-to-cache-any-number-of-endpoints
| } catch (e) { | ||
| console.log(e); | ||
| yield put(enqueueErrorSnackbar(i18n.t('message:ASSIGN_TASK.ERROR'))); | ||
| if (e.code === 'ERR_NETWORK') { |
There was a problem hiding this comment.
Our API going down (as it does, from time to time on release!) will also trigger ERR_NETWORK and therefore add that request to the workbox-background-sync queue. If this happens while the user is online, workbox-background-sync will also sync the queue immediately, which results in the SYNC_ITEM_FAILURE logical branch to be hit right away and two snackbars appearing:
I don't think this is ideal UX. We can't help the request being added to the queue in cases of API failure, but I think I would lean towards only showing the to-be-synced message when the user is offline? Maybe also with a service worker check like in the listener?
if (!navigator.onLine) {
yield put(enqueuePersistentSuccessSnackbar(i18n.t('message:TASK.UPDATE.SYNC.ONLINE')));
}This would mean just the network error snackbar showing. I'm not sure that's ideal UX either though, so I would love to discuss next week and resolve on the snackbar ticket.
There was a problem hiding this comment.
I'd still prefer not to show the to-be-synced message when the user is online 😂 But if we need to compromise, if (!navigator.onLine) seems reasonable!
There was a problem hiding this comment.
I'd still prefer not to show the to-be-synced message when the user is online
Yeah, I think you're right; this has to be the way!
But if we need to compromise,
if (!navigator.onLine)seems reasonable!
Sorry I might be missing something here... is there another way beyond the offline check? Or do you mean you would prefer to hide even the failure sync snackbar?
There was a problem hiding this comment.
Sorry for the confusion! I meant the offline check would be a good compromise if we need one; I didn't think about other options.
By the way, did you think about using isOffline state in the Redux store instead of navigator.onLine? I'm so obsessed decision making process in this PR 😂
There was a problem hiding this comment.
By the way, did you think about using isOffline state in the Redux store
Totally; I went back and forth and asked AI 😂 Actually I went against the recommendation which was to use Redux state! Because I liked the second and third pros so much:
Pros of navigator.onLine:
✅ Directness - No dependencies, no indirection
✅ Guaranteed fresh - Always current at the moment of check
✅ Simpler - No extra imports or selectors needed
But for completeness here were the cons which I ignored
❌ Inconsistency - Bypasses your existing offline system
❌ Duplication - Logic exists in two places
❌ Testability - Harder to mock navigator.onLine
It's not a super strong preference though; I'm also okay with
const isOffline = yield select(isOfflineSelector);
if (isOffline) {Do you like it better to have the single source of truth?
Edit: actually today I have decided I like it! Will update. (And sadly this is pretty representative of my decision-making process: ask AI, look at the two versions of the code, decide based on my feeling in that moment 😬)
| // 1. Immediately take control of the page | ||
| self.skipWaiting(); | ||
| clientsClaim(); | ||
|
|
There was a problem hiding this comment.
The first several lines of this file (up to the createOnSyncHandler()) take care of the same basic behaviour / static asset caching that the VitePWA-generated service worker did.
Requests are already pre-filtered by method since it's passed as the third argument to registerRoute
SayakaOno
left a comment
There was a problem hiding this comment.
Looks great! I really appreciate the detailed explanation and comments 😌
Most of my comments are just to strengthen my understanding.
There was a problem hiding this comment.
Do you think this would work as a hook? Now that I look at OfflineDetector, I think it should've been a hook too...
There was a problem hiding this comment.
Hmm I just noticed OfflineDetector is even in the hooks folder already! I totally missed that!
I think I had this weird idea that everything in App.jsx was supposed to be a component in the tree, probably because of <OfflineDetector /> 😂 But yeah these should definitely be hooks; they don't render! 🙏
Update: I don't know if this was a factor with <OfflineDetector /> (shouldn't matter unless the code was different once), but I see now one reason to use a component instead of a hook is to place the logic within the Suspense boundary. The service worker listener calls useTranslation() and actually does crash as a hook without moving the <Suspense />:
To fix the crash I moved the <Suspense /> up to main.jsx... it was a top-level Suspense in App anyway, so that should be equivalent, and it will leave open the option of calling more hooks in App in the future. But please let me know if that seems sound to you!
There was a problem hiding this comment.
Moving up the Suspense sounds good to me, although I haven't really worked with it myself...
The component approach seems like a good secret weapon to have; it was good learning, thank you!! 🙏
| type SyncArea = | ||
| | 'tasks.create' | ||
| | 'tasks.complete' | ||
| | 'tasks.abandon' | ||
| | 'tasks.update' // Generic fallback; use for patching date and assignee as well | ||
| | 'tasks.delete'; |
There was a problem hiding this comment.
I'm curious why you went with a type over enum. My go to is enum, so I'm just trying to learn pros and cons!
There was a problem hiding this comment.
Sure! My go-to is union of string literals, so I'd love to hear the reverse side too (why enums). I like the string literals just for developer experience reasons:
- I find lowercase easier to read quickly than all caps
- the code usually gets longer when having to include the enum name alongside each variable
- (not important here, but I think about this when it comes to props) I don't love having to import the enum into a new file in order to get the type checking and autocomplete
But that's about it. And I think I would prefer an enum too if the string name on its own didn't make sense without a grouping structure. Here the string matches the format of the sw exactly, so I felt it would be the opposite (kind of obfuscating to put into an enum).
I'm curious to learn why you lean towards enums in general!
There was a problem hiding this comment.
My mental model is pretty simple, I basically write enums the way I'd write const objects in JS, and I thought I had to use them for... no good reason 🤷♀️ I might have never even considered using union types! But I agree with all three points you mentioned about the downsides of enums, and I can't really find cons of using union types... Sorry, you're not learning anything new from me here 😅 I'll share if I come across good guidelines for when to use each one!
| client.postMessage({ | ||
| type: 'SYNC_ITEM_FAILURE', | ||
| payload: { | ||
| area, |
There was a problem hiding this comment.
Could we check the URL here to send the specific area (eg., tasks.complete, tasks.abandon) instead of resolving it later? I'm just wondering if that's an option.
There was a problem hiding this comment.
It's definitely an option! The only reason I put the parse in the listener, is that I feel a little more comfortable beefing up/extending the application code, rather than the service worker code. I don't have a super grounded reason for that preference, other than I'm just more familiar with working in the application context over the sw context, and it's easier to debug.
Does it read awkwardly to have the listener parse the URL?
There was a problem hiding this comment.
Not at all! That makes sense, the difference between service worker vs App context feels like a big difference, and debugging is what I didn't think of. Thank you 😄
| } catch (e) { | ||
| console.log(e); | ||
| yield put(enqueueErrorSnackbar(i18n.t('message:ASSIGN_TASK.ERROR'))); | ||
| if (e.code === 'ERR_NETWORK') { |
There was a problem hiding this comment.
I'd still prefer not to show the to-be-synced message when the user is online 😂 But if we need to compromise, if (!navigator.onLine) seems reasonable!
kathyavini
left a comment
There was a problem hiding this comment.
Thanks @SayakaOno! I converted both "components" to hooks and I added the offline check to the to-be-synched when online snackbars; I'll make sure to ask Loïc about the error one tomorrow.
I’ve added some context to the other questions below -- please let me know if those explanations/decisions make sense 🙏
There was a problem hiding this comment.
Hmm I just noticed OfflineDetector is even in the hooks folder already! I totally missed that!
I think I had this weird idea that everything in App.jsx was supposed to be a component in the tree, probably because of <OfflineDetector /> 😂 But yeah these should definitely be hooks; they don't render! 🙏
Update: I don't know if this was a factor with <OfflineDetector /> (shouldn't matter unless the code was different once), but I see now one reason to use a component instead of a hook is to place the logic within the Suspense boundary. The service worker listener calls useTranslation() and actually does crash as a hook without moving the <Suspense />:
To fix the crash I moved the <Suspense /> up to main.jsx... it was a top-level Suspense in App anyway, so that should be equivalent, and it will leave open the option of calling more hooks in App in the future. But please let me know if that seems sound to you!
| type SyncArea = | ||
| | 'tasks.create' | ||
| | 'tasks.complete' | ||
| | 'tasks.abandon' | ||
| | 'tasks.update' // Generic fallback; use for patching date and assignee as well | ||
| | 'tasks.delete'; |
There was a problem hiding this comment.
Sure! My go-to is union of string literals, so I'd love to hear the reverse side too (why enums). I like the string literals just for developer experience reasons:
- I find lowercase easier to read quickly than all caps
- the code usually gets longer when having to include the enum name alongside each variable
- (not important here, but I think about this when it comes to props) I don't love having to import the enum into a new file in order to get the type checking and autocomplete
But that's about it. And I think I would prefer an enum too if the string name on its own didn't make sense without a grouping structure. Here the string matches the format of the sw exactly, so I felt it would be the opposite (kind of obfuscating to put into an enum).
I'm curious to learn why you lean towards enums in general!
| client.postMessage({ | ||
| type: 'SYNC_ITEM_FAILURE', | ||
| payload: { | ||
| area, |
There was a problem hiding this comment.
It's definitely an option! The only reason I put the parse in the listener, is that I feel a little more comfortable beefing up/extending the application code, rather than the service worker code. I don't have a super grounded reason for that preference, other than I'm just more familiar with working in the application context over the sw context, and it's easier to debug.
Does it read awkwardly to have the listener parse the URL?
| } catch (e) { | ||
| console.log(e); | ||
| yield put(enqueueErrorSnackbar(i18n.t('message:ASSIGN_TASK.ERROR'))); | ||
| if (e.code === 'ERR_NETWORK') { |
There was a problem hiding this comment.
I'd still prefer not to show the to-be-synced message when the user is online
Yeah, I think you're right; this has to be the way!
But if we need to compromise,
if (!navigator.onLine)seems reasonable!
Sorry I might be missing something here... is there another way beyond the offline check? Or do you mean you would prefer to hide even the failure sync snackbar?
| "SYNC": { | ||
| "FAILED": "Task changes failed to save", | ||
| "LOCATION_DELETED": "Task failed to save: location has been retired", | ||
| "NETWORK_ERROR": "Unable to reach server. Will retry automatically", |
There was a problem hiding this comment.
"Sorry we are down momentarily, but will retry automatically"
"Sorry we are down momentarily, but will retry your task update/creation automatically"
Restore different messages depending on endpoint; adjust wording for all according to dev-design discussion
|
@SayakaOno I think everything that we discussed this morning has been covered and it should now be good for re-review 🙏 Thank you for your help! Two small notes:
Of course it would still be really good to test this all on beta with devices actually disconnected! |
| if (swContainer) { | ||
| swContainer.addEventListener('message', handleServiceWorkerMessage); | ||
| window.addEventListener('online', replayQueue); | ||
| } |
There was a problem hiding this comment.
This looks like a duplicate, should it be removed?
Other changes look good to me! Thank you so much for catching that the Background Sync API isn't supported in all browsers!
There was a problem hiding this comment.
Oooh thank you for catching that ❤️
SayakaOno
left a comment
There was a problem hiding this comment.
The code looks great! Let's test it on beta!
FYI - I couldn't get it working locally on Firefox... File > Work Offline showed the offline bar, but API requests still work 😅
😳 Wait you're totally right. OK I was messing around in Firefox for a while so it might have been some other combination 😂 Buuuut... just saw a synced snackbar on beta 😁😮💨 |
Description
This PR adds
workbox-background-syncqueuing to all the routes matching/task/. The functionality has three parts which I'll elaborate on below.The custom service worker
(I'm still a complete novice when it comes to service workers, so I hope this explanation is accurate and clear! Please do tell me if I'm missing something 🙏)
Our new custom servicer worker replaces the
sw.jsthat used to be created automatically by VitePWA.At the bottom of the file we import
workbox-background-syncas a plugin, and register the routes we want queued. Defining routes to register can be done without using a custom service worker at all (i.e. using VitePWA'sgenerate-swstrategy as shown in the POC branch in comments here), but we are using the custom service worker in order to completely replace the defaultonSynchandler with our modified version.Our
onSynchandler for each queue, in addition to running through the entries in the normal way, also uses the service worker-native method client.postMessage() to share informative messages about each request as it's synced. It's just for this message-posting functionality that we are using a custom service worker at all.If you have any interest in debugging in the service worker to look around and see what else is available within the workbox-background-sync
onSynccallback, I'll leave additional notes below specifically about service worker testing. Theworkbox-background-syncqueue items can additionally always be manually inspected withinIndexDB.Note: I couldn't find any documention on how to format an
onSynchandler -- the code links to the actual class for the workbox Queue, because this was the file referenced by the AI model (I think o4-mini?) who located it when I was building the POC branch, and I haven't found anything better since. Theworkbox-background-syncdocumentation site seems too minimal to me; it does mentionqueue.shiftRequest()andqueue.unshiftRequest()methods, but doesn't have anything in terms of putting it all together!The Listener Component
The
<ServiceWorkerListener />component listens for the messages posted withclient.postMessage():and then handles them according to rules set up for each endpoint within
syncConfig.The
syncConfigis organized by area of the application (= the route), and then each area is defined with: a) the snackbar to display on success, b) the snackbar(s) to display on API error responses according to their status code, c) any side effects to run ononSuccess(), and finally the refresh action which is alwaysgetTasks()for this part of the application. The side effects come from their respective sagas and were thankfully quite minimal!If the queue item cannot be cleared (i.e. the response is another
ERR_NETWORK) theSYNC_ITEM_FAILUREmessage is emitted and the listener shows the "network error" snackbar. These items are not cleared fromworkbox-background-syncqueue, but rather will be re-tried with a timing logic that the browser controls. For local testing, you can see this logical branch by shutting down the API.Saga
The code changes in the saga are the least important 🙂 The saga has absolutely nothing to do with requests being added to the queue; once the
workbox-background-syncplugin is active, network errors result in requests being added to the queue at the browser level -- completely outside the scope of our app. The code in the saga therefore just 1) shows the snackbar and 2) applies the optimistic update in Redux where applicable. There is a dedicated optimistic update ticket (linked in comments) to refine that part.Jira link: https://lite-farm.atlassian.net/browse/LF-5104 & https://lite-farm.atlassian.net/browse/LF-5121 & https://lite-farm.atlassian.net/browse/LF-5115
Type of change
How Has This Been Tested?
To test the service worker functionality, it is unfortunately always necessary to do
pnpm buildandpnpm preview(which makes testing code changes painfully slow!) To test the offline behaviour, make sure that"Network request blocking"is checked within the Chrome dev tools underSettings > Preferences > NetworkTo debugger in the service worker file, you will need to open a dedicated dev tools instance by selecting
inspectnext to the service worker here:Checklist:
pnpm i18nto help with this)