[Do not merge, discuss after 1.0.0] Service Worker added for offline documentation #247
Conversation
Nooshu
requested review from
36degrees,
alex-ju,
NickColley,
kr8n3r and
hannalaakso
|
You can preview this change here: Built with commit d6ccf3e
https://workbox-service-worker--govuk-design-system-preview.netlify.com/ |
Nooshu
force-pushed
the
workbox-service-worker
branch
2 times, most recently
from
9e80a74
to
78af22c
Compare
|
Hey Matt, thanks for this awesome contribution. What network strategy does workbox use? I imagine we'd want a 'network falling back to cache' approach? This ensures people always see the most up-to-date content. Stale content could refer to older versions of GOV.UK Frontend which would be problematic. |
|
Good point @NickColley, I'll add in the |
workbox/workbox-cli-config.js
Outdated
| globPatterns: [ | ||
| '**/*.{html,js,css,png}' | ||
| ], | ||
| swDest: paths.public + 'sw.js', |
There was a problem hiding this comment.
Nit: I know it's common for 'sw' to be used in tutorials and such but we prefer to avoid abbreviations. Could anywhere there's SW could we use 'ServiceWorker' instead?
There was a problem hiding this comment.
💯 This is a great feature for the Design System.
I'll leave a first comment about the file structure (I'm consciously being picky):
workbox-cli-config.jsshould be in/configbuild-service-worker.jsshould be in/lib
I'm happy to do the above changes myself if they're agreed.
I'll try and put more thoughts into the build integration (potentially with injectManifest) the next days.
|
Placement of files has now been updated. Both elements and frontend are served from the same port on localhost, so when switching between projects the service worker clashes. I've updated the frontend port for the moment, but we could consider changing elements port instead? Now explicitly using |
config/workbox-cli-config.js
Outdated
| module.exports = { | ||
| globDirectory: paths.public, | ||
| globPatterns: [ | ||
| '**/*.{html,js,css,png}' |
tasks/serve.js
Outdated
|
|
||
| // setup synchronised browser testing | ||
| metalsmith.use(browsersync({ | ||
| open: false, | ||
| open: true, |
There was a problem hiding this comment.
What does this flag do, why is changed?
There was a problem hiding this comment.
It was originally true (in master). I have reverted it back to true here. It opens up a new browser window every time you npm start. This was becoming insanely annoying as I was testing so I disabled it.
Nooshu
force-pushed
the
workbox-service-worker
branch
from
858811f
to
cc89529
Compare
|
I might be being too cautious but I'd like to try the following before approving:
|
|
Looks good based on those tests above, we noticed that there's a runtime cache that caches Analytics. If possible it would be good to avoid this, as it might make Google Tag Manager unpredictable. |
Nooshu
force-pushed
the
workbox-service-worker
branch
from
a966ebe
to
d6ccf3e
Compare
|
I've been unable to exclude analytics from the caching. This looks to be something Workbox does automatically. What I have done is set the caching strategy on those files to networkFirst and set an expiry of 1 hour so they will always be up to date. I asked in the Workbox repo about the runtime caching. Sounds like setting a short expiry for these files is a good strategy. |
| } | ||
| }, | ||
| { | ||
| urlPattern: new RegExp('https://www.googletagmanager.com/gtm.js'), |
There was a problem hiding this comment.
Wondering if we could do /https:\/\/www\.google.*\.com\/.*\.js/ here instead, which matches any javascript file from a domain that starts with google?
If that's no good, this seems fine for me my main worry is that these URLS can change since they're injected at runtime.
There was a problem hiding this comment.
Sure, we can do that. I was trying to be explicit as I'm unsure what other Google URL's we may be using in the future.
There was a problem hiding this comment.
Let's go with this for now then. 👍
| ], | ||
| swDest: paths.public + 'service-worker.js', | ||
| importWorkboxFrom: 'local', | ||
| skipWaiting: true, |
There was a problem hiding this comment.
Sorry didnt notice this earlier, should we have skipWaiting and clientsClaim here? I was under the impression these are used in development and break out of the service worker lifecycle?
There was a problem hiding this comment.
Think this may be OK since we're doing a network first approach
There was a problem hiding this comment.
The waiting phase is useful when you're only running one version of your site at once, but since we don't need that feature we can make the new service worker activate sooner, so I think setting it to true is good.
| ], | ||
| swDest: paths.public + 'service-worker.js', | ||
| importWorkboxFrom: 'local', | ||
| skipWaiting: true, |
There was a problem hiding this comment.
Think this may be OK since we're doing a network first approach
|
Any feedback on this PR @36degrees? Don't want to merge until you've looked over it. |
There was a problem hiding this comment.
I definitely have some reservations about merging this.
I am excited by the possibilities – and I think it could be really useful, but I don't think that it's a feature we would prioritise ourselves at this stage, and so if we were going to merge this I think we would want to be confident that it wasn't going to cause us any problems down the line.
It's adding a reasonable amount of extra complexity which I don't think is well understood by the team.
From testing, Chrome downloads 23.1MB of data on first page load (as tested in incognito mode). Would this also happen if a user visits from a mobile phone on their perhaps-limited data plan? I don't think that's OK.
I don't think we can be confident that this is 'just going to work' and I don't think we want the overhead of manually 'testing' it regularly either. The trailing slash issue I've raised as a comment is a good example, but to provide another – from reading the config I think that if we started using SVGs to illustrate patterns they would not be cached by the service worker, so the user would get a broken experience. I do not think we would catch this. I think we need to be more confident in the approach and/or that we would be able to catch problems (e.g. using automated testing)
We don't understand the implications that this will have on analytics - how will offline views be recorded? If users 'come back online' at a later date (e.g. they come online on a Monday after being offline whilst travelling on a Friday) will the statistics for the Friday then change 'retrospectively'? Is this behaviour something that's going to cause headaches for the performance analysts?
Personally, I don't know enough about Workbox or service workers to be sure that my concerns are exhaustive – I think there may be other implications (especially in terms of performance and security) that we haven't considered.
There's a lot of work still to do here and I don't think now is the right time to be doing it.
| @@ -2,11 +2,11 @@ | |||
| <ul class="app-mobile-nav__list"> | |||
| {% for item in navigation %} | |||
| <li> | |||
| <a class="govuk-link js-mobile-nav-subnav-toggler" href="/{{ item.url }}">{{ item.label }}</a> | |||
| <a class="govuk-link js-mobile-nav-subnav-toggler" href="/{{ item.url }}/">{{ item.label }}</a> | |||
There was a problem hiding this comment.
I am guessing that this change (and the other trailing slash changes) is attempting to fix an issue with resolving trailing-slash-less paths when falling back to the service worker's cache?
I don't think we can rely on this – as well as the navigation there are links from components and patterns to other related components and patterns, etc. As an example, there's a link from the error summary component to 'error messages' (under 'How it works') which doesn't end in a trailing slash, and which is broken when offline.
I don't think 'enforcing' trailing slashes is really enforceable, and I don't think we can rely on regularly testing this functionality either – is there a way we can modify the service worker to resolve either way?
There was a problem hiding this comment.
This is certainly something that can be investigated further. Without a trailing slash the SW will assume /about is referring to /about.html. It looks for the trailing slash before appending the directoryIndex setting. There is a prettyUrl setting that didn't fix the issue. There is a promising comment here that we can look at.
I think as @alex-ju mentioned above we should switch to the injectManifest method, this gives us a lot more fine grained control over how the SW works.
There was a problem hiding this comment.
This issue has now been fixed in the service worker without the need for modification.
Yes, very good point. And this raises questions on how SW's actually handle this issue. I haven't seen a setting that only allows downloading on a wi-fi network rather than a data plan. This also raises the question of what experience do we want an offline user to have? Is it better to have no offline experience at all than a slightly broken one (missing images for example)? Personally I was thinking it would be useful to have the full set of documentation to refer to while working remotely (commuting on a train for example), but maybe that isn't the case? We have the ability to define routes and include / exclude any type of asset, as well as defining different caching strategies where required. It also raises the question of what is taking up the 23.1MB and can this be optimised in anyway.
At the moment offline tracking isn't enabled. If this is a requirement then there is a module in Workbox that can be enabled. A custom dimension would be setup in GA so we could distinguish between and online / offline user. |
|
Two main problems that are causing the increased cache size is:
The Base64 encoded fonts are 200KB. After investigation changing the font loading strategy reduces the cache size by 72%. Duplicated assets still need to be investigated. |
Nooshu
changed the title
|
Closing for the moment. Will reopen again once I have an update. |
|
Adding a small comment from x-gov slack. Gavin Wye from MoJ asked:
Looks like there is a user-need for this in the future. |
|
There has been significant progress with this branch as the following changes made by the DS team have really helped reduce the overall size of the asset precaching:
The fonts are no longer included via Base64 encoding, and assets that were repeated across the codebase multiple times have been reduced almost completely.
All these changes have had a huge effect on the amount of data being pre-cached. A reduction of 79%, from 23.1MB to 4.9MB. This is to store the whole of the Design System documentation and assets for offline usage.
The largest single asset that is now being pre-cached is the
A preview of the Design System using the Service worker and offline mode can be viewed here. |
|
@Nooshu it looks like the assets are not being GZIPed? |
|
I think we'd want to have a simple test plugin that checks the assets in the workbox manifest for sanity and breaks the build if it is very odd. |
Addressed the duplicated images in #315 |
|
Rebased #315 and can confirm the duplicate images have been removed from the service worker manifest. The CSS files still exist:
|
Nooshu
changed the title
|
Since we're moving to GOV.UK/Design System we'd need to be very careful that this Service Worker is only scoped to /design-system |
|
After discussion with @igloosi and testing in a new incognito window it can be seen that the data that is transferred across the network is 1.4MB in total (214 files transferred, 201 files cached): This is then cached and uncompressed to around 7.6MB in the browser: These figures have been confirmed on multiple devices. |
|
@NickColley good point about the URL, I'll add that to the list now. Update: Scope of the SW has now been restricted to the path the service worker sits in. |
|
500KB limit has now been added to |
@NickColley I'e added a very simple set of checks that output a warning to the console if the total file size or the total number of files increases over a certain threshold. The previous state is written to a file in the filesystem, this seemed like a fairly simple way to store this state, but I'm open to ideas of how to improve this. It doesn't fail the build at the moment, it just shows a warning (screenshot below).
|
|
Removed the need to use the Google CDN to serve Workbox. Following the guide here and using the CLI command: |
|
Started to investigate the use or routing to allow different caching strategies to be used and a different cache per type of asset. Commit investigation here Only the HTML files are being precached, with all other assets sitting in a separate cache.
UPDATE: Fonts cache now has a 7 day expiry added using the |
|
Added offline detection functionality. When a users connection is dropped a small red tag will be shown in the bottom right corner of the DS. This is to tell the user they are using the cached version of the DS. This tag will be removed when the connection is reestablished. The design uses the same styles as the "beta" tag only with a different BG colour. I expect the design to be changed and the actual code structure to be changed in the future as required. See screenshot for a preview:
|
|
|
I've just reviewed the latest setup with all the 1.0.0 changes and HTML only being precached. With GZip compression enabled the total initial download with the SW enabled is 848KB. This setup allows a user to access all pages offline, all functional with the fonts and JS. There may be a few images missing since these aren't being precached.
|
|
I've been investigating the possibility of implementing a "kill switch" for the service worker. This is in the unlikely event that something odd happens in the SW and there is a need to kill it in client browsers, as this isn't something we would expect a user to do. Fortunately this requirement is already part of the service worker specification as of 2016. Essentially the service worker JS file sits outside of the standard browser cache. When a user navigates to a page that sits under the service worker scope or when the service worker is woken up, it will make a request to the server and look for an updated service worker JS file. If it finds an update it will update this in the background without the user even knowing it happened. There is a great write-up here by Jeff Posnick:
It is still recommended that a |
In a recent document discussing the "Design System and Frontend documentation" there was a question about where the documentation should sit. One of the options was "All documentation lives in the Design System and the main Frontend README is just a link to the Get started guide in the Design System". The only risk listed with this approach was "Users who clone the repo or download the zip from GitHub won’t have access to offline documentation." This PR has been created to solve this issue for Design System users who require documentation while they are offline.
A service worker has been added with all the heavy lifting done using Workbox. Workbox is built into the metalsmith build pipeline. Workbox handles the file manifest and looks for changes between builds. Once a change is detected, the manifest is updated. Old cache is cleared and replaced with the newest version.
It is now possible to view all the DS docs while offline. There may be considerations around when this PR is merged as a service worker does add another level of caching that can sometimes complicate development. The level of debugging available in the console for 3.1.0 of Workbox has been vastly improved so a developer gets a much clearer picture of what is happening. We could hide all this behind a 'development' / 'production' check or even a simple boolean to disable when required.
Current issues
Todo