-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
feat(gatsby-plugin-offline): replace no-cache detection with dynamic path whitelist #9907
Conversation
Manual tests all passing for configuration 2.1 (commit 465a4d2) |
Manual test 3A failing for configuration 2.2 (commit 465a4d2) |
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 looking good! Just left a little comment, but the IndexedDB approach seems solid, particularly because that was the original Workbox recommendation.
Before we get this merged, let's see if we can build out a site with the service worker (i.e. gatsbyjs.org?) and validate some things manually, although it does seem like you have a good manual test strategy currently!
self.addEventListener(`message`, event => { | ||
const { api } = event.data | ||
if (api === `gatsby-runtime-cache`) { | ||
importScripts( |
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.
I thought we were gonna go a configurable whitelist approach instead of the indexeddb approach? I'm fine with either, just want to set expectations so I'm aware.
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.
I tried that at first (with the first two commits of this PR) but realised it wouldn't be a great solution given that some sites might depend on the existing functionality - this is a much more reliable solution since it only whitelists pages whose resources have successfully downloaded, so doesn't e.g. prevent Netlify CMS from working.
If we make assumptions about the default whitelist then it would make things worse for people who put Gatsby pages in places like /admin (maybe they're making a custom admin panel), since these pages wouldn't work offline.
Also, just to clarify in case there's any confusion - the original idea was to use the IDB to replace ?no-cache=1 by temporarily blacklisting pages which are detected to not be Gatsby. While this approach uses IDB, it works in the opposite way by only whitelisting pages on which Gatsby is detected, since this is easier to do and safer. (Sorry if you were already aware of that!)
Manual tests all passing for configuration 2.2 (commit c62c378) |
Manual tests all passing for configuration 1.1 (commit c62c378) |
Manual tests all passing for configuration 1.2 (commit c62c378) |
Manual tests all passing for configuration 3.1 (commit c62c378) |
Manual tests all passing for configuration 3.2 (commit c62c378) |
Manual tests all passing for configuration 4.2 (commit c62c378) |
Manual tests all passing for configuration 4.1 (commit c62c378) |
Going to give a thorough review, but first a quick question: On #9415, how were you able to validate that this PR fixes? As best as I can recall, you couldn't ever reproduce the issue right? |
I haven't actually verified this yet, but I think based on what some others said, the problem was caused by these lines in
I'll see if I can reproduce the cache issue to test this properly, but I might need someone else to help if I can't do so (maybe I'll be able to on Windows or macOS). |
@davidbailey00 I can reproduce it, so I can validate whether this does (or doesn't!) fix 🎉 I can't guarantee I'll get to it today, but I'll try! |
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.
I'm gonna approve this. I tested it out locally and it seems to work just fine, but it's fairly challenging to validate the things that this purports to fix unfortunately.
Going forward - I think it's crucial we get an e2e test suite set up that validates offline functionality so that any future changes to this plugin validate against a base set of functionality without manual testing. I know you're already on that though!
…path whitelist (gatsbyjs#9907) * Remove all no-cache code * Remove references to no-cache in offline plugin * Initial work on hybrid navigation handler * Refactor whitelist code to allow it to support onPostPrefetchPathname * Fix service worker detection * Fix IndexedDB race condition * Prevent race conditions + reset whitelist on SW update * Remove unnecessary API handler (onPostPrefetchPathname is called anyway) * Add debugging statements + fix some minor problems * Fix back/forward not working after 404 * Remove unneeded debugging statements * Bundle idb-keyval instead of using an external CDN * Update README * Backport fixes from gatsbyjs#9907 * minor fixes for things I copy-pasted wrong * Refactor prefetching so we can detect success
Fixes #9355, fixes #9415, fixes #9661, fixes #9939
Changes made:
gatsby-plugin-offline
gatsby-browser.js
gatsby-node.js
sw-append.js
idb-keyval-iife.min.js
to thepublic
folder when buildingsw-append.js
README.md
gatsby/cache-dir
ensure-resources.js
load-directly-or-404.js
codeload-directly-or-404.js
- delete itloader.js
stripPrefix
code (the prefix is already stripped so it caused errors)failedResources
object (its values are never read)jsonPromiseStore
if the resources fail to loadfetchResource
in the console, rather than silently catchingonPostPrefetchPathname
fromgetResourcesForPathname
andenqueue
if they were successful (as per the docs, it should only be called upon success)prefetch.js
navigation.js
load-directly-or-404.js
codeproduction-app.js
load-directly-or-404.js
codeTODO:
onPostPrefetchResources
can be called fromenqueue
Fix resources failing to load after going offline, then coming online again and hovering over linkssee Not recovering from failed dynamic import webpack-contrib/mini-css-extract-plugin#278Manual tests: