Handle/cache offline-capable mode media requests in service worker #465
Conversation
Codecov Report
@@ Coverage Diff @@
## master enketo/enketo-express#465 +/- ##
==========================================
+ Coverage 62.63% 63.07% +0.44%
==========================================
Files 52 52
Lines 3669 3648 -21
==========================================
+ Hits 2298 2301 +3
+ Misses 1371 1347 -24
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
I intended to add integration tests for the service worker today, but I don’t know how realistic that is without additional tooling or an elaborate test server setup and careful teardown considerations for the client to send requests. Unrealistic, because the service worker global scope is isolated from the main thread so its I’d also considered directly unit testing the event handlers, but I think this is a rare case where there’s nothing of value to unit test. The integration test value would validate assumptions about the browser API behaviors and that they match expected behavior. Unit tests in this case would only validate that my assumptions are what I said they are. There are great tooling options if we decide to go that route here or later. Off the top of my head: Cypress, Puppeteer and Playwright are all good potential options. There are also WebDriver solutions in some examples which I’d prefer to avoid if possible. For now I’m inclined to accept this lack of new test coverage, and take the big wins this PR already brings, and the foundation it lays for other big wins described in enketo/enketo#990. I’ve manually verified the heck out of this, and feel more confident working in a variety of offline-specific code as a result. So I’m gonna mark this one ready for review! |
eyelidlessness
force-pushed
the
refactor/media
branch
from
f2cf5c3
to
32673bd
Compare
eyelidlessness
force-pushed
the
refactor/media
branch
4 times, most recently
from
44d6e0f
to
6427da3
Compare
tutorials/37-offline.md
Outdated
| #### Form and form attachments | ||
| The resources in this cache will be updated when Enketo detects that they have changed. | ||
|
|
||
| For production Enketo deployments, these resources will always be retrieved from the cache if present, without performing additional network requests other than to determine whether they have been updated. To aid maintenance and improvement of Enketo's offline functionality, the requests _are_ performed in development mode. |
There was a problem hiding this comment.
Help me get the flow in more detail here. I'm online, I go to an offline-capable form. There's no Enketo update but there are form updates. Here's what I believe happens and as I understand it this is unchanged by this PR:
- The form loads immediately from cache
- Enketo polls the form server and determines there's an update (either of the form definition or of anything in the manifest or both)
- Enketo shows the banner prompting me to refresh
- When I refresh, Enketo requests the updated form and form attachments. It requests all of them even if only one of them has changed. E.g if only the form has changed, attachments are still updated. If only a 200kb attachment has been updated, it still downloads that and the 60MB video that wasn't updated.
Why is there a delay in the version change polling? Is that something Enketo-specific or just the APIs?
There was a problem hiding this comment.
Actually, I don't think that's what happens anymore. Here's now what I'm thinking is going on. I think this is "similar but additive change was made to check for form updates on page load as well." from the PR description
Before:
- The form loads immediately from cache
- After a short delay, Enketo polls the form server and determines there's an update (either of the form definition or of anything in the manifest or both)
- Enketo shows the banner prompting me to refresh
- When I refresh, Enketo requests the updated form and form attachments. It requests all of them even if only one of them has changed. E.g if only the form has changed, attachments are still updated. If only a 200kb attachment has been updated, it still downloads that and the 60MB video that wasn't updated.
After:
- The form does not load immediately
- Enketo determines there's an update (either of the form definition or of anything in the manifest or both)
- Enketo requests the updated form and form attachments. It requests all of them even if only one of them has changed. E.g if only the form has changed, attachments are still updated. If only a 200kb attachment has been updated, it still downloads that and the 60MB video that wasn't updated.
- The form loads
There was a problem hiding this comment.
For form updates, the flow you describe here is almost right. When a change is detected, the form cache is fully updated in the background before the banner is shown. Once prompted, if the user reloads they'll again request the form from the cache. A substantive change in this PR is that the initial check is performed immediately without a delay. (Currently this is blocking, but that was a mistake I'll correct shortly.)
Likewise, a substantive change in this PR is that the initial check for service worker updates is no longer delayed (and also non-blocking). There's an inherent delay for service worker updates because once one has been registered it's active immediately on load—i.e. before an updated one can be registered, installed and activated.
I think we should also maybe limit the initial check auto-reload to a shorter time span (e.g. 500ms-1s?), in case for some reason the service worker update takes longer and would potentially interrupt the user's work. (But maybe auto-save adequately addresses this?)
There was a problem hiding this comment.
the form cache is fully updated in the background before the banner is shown
Nice!
this is blocking, but that was a mistake I'll correct shortly
Ok, great. I'm not entirely sure which is best but seems reasonable to leave close to the current behavior. The improvement is that the banner will show up right away if on a fast connection. The pause is disconcerting, I find.
I think we should also maybe limit the initial check auto-reload to a shorter time span (e.g. 500ms-1s?
What is the user experience of the auto-reload? Does it feel like a visible page refresh? There's currently no limit, right? Going to a banner experience after a second feels like a reasonable approach.
There was a problem hiding this comment.
Does it feel like a visible page refresh?
Yes and it's weird.
Follow-on work would be to move the service worker registration earlier in the flow to avoid the double reload.
|
Do the changes to the service worker account for the config changing independent of the build? That is, clients of a production server would get updates in that case? #382 (comment) |
In conversation, @eyelidlessness says yes. The prior behavior was that changing the config required a service restart (but not rebuild) and that is preserved. Let's add that to the docs! |
tutorials/37-offline.md
Outdated
| #### Form and form attachments | ||
| The resources in this cache will be updated when Enketo detects that they have changed. | ||
|
|
||
| For production Enketo deployments, these resources will always be retrieved from the cache if present, without performing additional network requests other than to determine whether they have been updated. To aid maintenance and improvement of Enketo's offline functionality, the requests _are_ performed in development mode. |
There was a problem hiding this comment.
Actually, I don't think that's what happens anymore. Here's now what I'm thinking is going on. I think this is "similar but additive change was made to check for form updates on page load as well." from the PR description
Before:
- The form loads immediately from cache
- After a short delay, Enketo polls the form server and determines there's an update (either of the form definition or of anything in the manifest or both)
- Enketo shows the banner prompting me to refresh
- When I refresh, Enketo requests the updated form and form attachments. It requests all of them even if only one of them has changed. E.g if only the form has changed, attachments are still updated. If only a 200kb attachment has been updated, it still downloads that and the 60MB video that wasn't updated.
After:
- The form does not load immediately
- Enketo determines there's an update (either of the form definition or of anything in the manifest or both)
- Enketo requests the updated form and form attachments. It requests all of them even if only one of them has changed. E.g if only the form has changed, attachments are still updated. If only a 200kb attachment has been updated, it still downloads that and the 60MB video that wasn't updated.
- The form loads
eyelidlessness
force-pushed
the
refactor/media
branch
2 times, most recently
from
8687ab1
to
aff4108
Compare
This only affects dev, but otherwise requires a server restart to make any service worker changes
- Claim clients immediately on activation. It's not clear this completely eliminates the need for a reload, but it does get part of the way there. - Don't attempt to cache responses for non-GET requests
Also, no reason for an array of keys which only ever has a length of one
This should not vary between different forms. This actually fixes a bug in both the current implementation as well as the previous changes on this branch: 1. Request offline-capable Form A while online 2. Request offline-capable Form B while online 3. Any changes which would trigger a service worker update 4. Request offline-capable Form A again while online, which updates the service worker and wipes out the previous cache 5. Request offline-capable Form B while OFFLINE, which will fail because its cached HTML has been removed Unless or until there is some meaningful difference in the HTML served for different forms, this change should be safe because the HTML and service worker will both be re-cached at the same time
# Conflicts: # tutorials/37-offline.md # Conflicts: # tutorials/37-offline.md
And unnecessary try/catch block around initial non-blocking servicer worker update check
eyelidlessness
force-pushed
the
refactor/media
branch
from
a59625c
to
1c39cec
Compare
Part of enketo/enketo#990. Closes enketo/enketo#1017. This includes several related changes/improvements:
resources(themes and icon) can be prefetched using theLinkheader. Thiswillmay eventually also enable adding test coverage for the service worker.I have verified this PR works with
What else has been done to verify that this works as intended?
Copious manual testing, new automated testing of the service worker registration flow.
Why is this the best possible solution? Were any other approaches considered?
Uses well established web standards to handle one of the Hard Problems. Eliminates the vast majority of incidental differences between online-only and offline-capable modes, and enables elimination of nearly all of those differences which aren’t essential. Also makes unifying the app entry points, and adding test coverage for them, almost trivial. The only realistic alternative option would be to leave on the table all of the benefits this brings for maintenance and confidence delivering features more safely and quickly.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
It’s possible there’s an oversight in terms of out of band offline media logic in enketo-core, but I’ve exhausted every case I can think of. As other parts of enketo/enketo#990 progress those chances diminish more.
Do we need any specific form for testing your changes? If so, please attach one.
No specific form, but this should be tested against any thing affected by form and instance attachments.