Skip to content
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

Merged
merged 26 commits into from Nov 20, 2018
Merged

feat(gatsby-plugin-offline): replace no-cache detection with dynamic path whitelist #9907

merged 26 commits into from Nov 20, 2018

Conversation

valin4tor
Copy link
Contributor

@valin4tor valin4tor commented Nov 13, 2018

Fixes #9355, fixes #9415, fixes #9661, fixes #9939

Changes made:

  • gatsby-plugin-offline
    • gatsby-browser.js
      • Detect if the SW is installed with a more reliable method
      • Whitelist pathnames which have been prefetched
    • gatsby-node.js
      • Remove navigate fallback options
      • Move the catch-all Network First handler to sw-append.js
      • Copy idb-keyval-iife.min.js to the public folder when building
    • sw-append.js
      • Define a custom navigation route, which serves the offline shell for whitelisted paths
      • Refactor the API to separate individual functions from the message handler
      • Add new APIs to whitelist a path, and to reset the whitelist
    • README.md
      • Update default options
      • Remove reference to AppCache (replaced by service workers)
  • gatsby/cache-dir
    • ensure-resources.js
      • Remove all load-directly-or-404.js code
      • Upon a resource load failure, reload the page in order to display a server 404 or offline error
    • load-directly-or-404.js - delete it
    • loader.js
      • Remove unnecessary stripPrefix code (the prefix is already stripped so it caused errors)
      • Remove unnecessary failedResources object (its values are never read)
      • Delete promises from jsonPromiseStore if the resources fail to load
      • Show errors from fetchResource in the console, rather than silently catching
      • Use object shorthand notation where possible
      • Only call onPostPrefetchPathname from getResourcesForPathname and enqueue if they were successful (as per the docs, it should only be called upon success)
    • prefetch.js
      • Detect whether prefetching was successful
    • navigation.js
      • Remove all load-directly-or-404.js code
      • Reset the IDB whitelist if the service worker has updated
    • production-app.js
      • Remove all load-directly-or-404.js code

TODO:


Manual tests:

configurations:

- 1) online without gatsby-plugin-offline
- 2) online with gatsby-plugin-offline
- 3) offline with gatsby-plugin-offline (start on /about to prevent all URLs prefetching)
- 4) online in develop

- x.1) with a custom 404 page
- x.2) without a custom 404 page

tests:
- 1) 404s are handled correctly from internal links - custom 404 or server 404
  if custom is unavailable, or offline error, but no blank pages
- 2) 404s are handled correctly when entering a URL directly - as above, and
  the URL fetched contains the original query parameters (not just no-cache=1)
- 3A) back button works properly after internal link 404 (navigates to the
  expected page)
- 3B) back button works properly after external link 404 (navigates to the
  expected page)
- 4) pages which fail to load display a native offline error (no blank pages)
- 5) the service worker remains installed after a 404/offline error
- 6) Netlify CMS /admin/ loads when reloaded / URL directly entered (or offline
  error displayed)
- 7) going to a page by entering the URL directly works
- 8) directly entering the mismatching canonical URL works with JS, and goes
  backwards and forwards correctly (or shows offline)

@valin4tor valin4tor requested a review from a team as a code owner November 13, 2018 17:33
@valin4tor
Copy link
Contributor Author

valin4tor commented Nov 13, 2018

Manual tests all passing for configuration 2.1 (commit 465a4d2)

@valin4tor
Copy link
Contributor Author

Manual test 3A failing for configuration 2.2 (commit 465a4d2)

Copy link
Contributor

@DSchau DSchau left a 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(
Copy link
Contributor

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.

Copy link
Contributor Author

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!)

@valin4tor
Copy link
Contributor Author

Manual tests all passing for configuration 2.2 (commit c62c378)
Configuration 2.1 unaffected

@valin4tor
Copy link
Contributor Author

valin4tor commented Nov 13, 2018

Manual tests all passing for configuration 1.1 (commit c62c378)
Configurations 2.1 and 2.2 unaffected

@valin4tor
Copy link
Contributor Author

Manual tests all passing for configuration 1.2 (commit c62c378)
Configurations 1.1, 2.1 and 2.2 unaffected

@valin4tor
Copy link
Contributor Author

Manual tests all passing for configuration 3.1 (commit c62c378)
Configurations 1.1, 1.2, 2.1 and 2.2 unaffected

@valin4tor
Copy link
Contributor Author

Manual tests all passing for configuration 3.2 (commit c62c378)
Configurations 1.1, 1.2, 2.1, 2.2 and 3.1 unaffected

@valin4tor
Copy link
Contributor Author

Manual tests all passing for configuration 4.2 (commit c62c378)
Configurations 1.1, 1.2, 2.1, 2.2, 3.1 and 3.2 unaffected

@valin4tor
Copy link
Contributor Author

Manual tests all passing for configuration 4.1 (commit c62c378)
Configurations 1.1, 1.2, 2.1, 2.2, 3.1, 3.2 and 4.2 unaffected

@valin4tor valin4tor changed the title [WIP] feat(gatsby-plugin-offline): replace no-cache detection with dynamic path whitelist feat(gatsby-plugin-offline): replace no-cache detection with dynamic path whitelist Nov 13, 2018
@DSchau
Copy link
Contributor

DSchau commented Nov 13, 2018

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?

@valin4tor
Copy link
Contributor Author

On #9415, how were you able to validate that this PR fixes?

I haven't actually verified this yet, but I think based on what some others said, the problem was caused by these lines in gatsby-browser.js:

image

swNotInstalled was true for any subsequent pageloads, because it's only made false immediately after the SW has installed - it should always be false once the SW has finished installing. I changed the detection method, so it detects properly whether the SW has installed.

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).

@DSchau
Copy link
Contributor

DSchau commented Nov 13, 2018

@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!

Copy link
Contributor

@DSchau DSchau left a 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!

@valin4tor valin4tor merged commit 8d3af3f into gatsbyjs:master Nov 20, 2018
@m-allanson m-allanson added this to Done in OSS Roadmap Jan 7, 2019
gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
OSS Roadmap
  
Done
2 participants