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

[gatsby-plugin-offline]: Potential for caching problems with idb-keyval #24936

Closed
nibtime opened this issue Jun 11, 2020 · 0 comments · Fixed by #24938
Closed

[gatsby-plugin-offline]: Potential for caching problems with idb-keyval #24936

nibtime opened this issue Jun 11, 2020 · 0 comments · Fixed by #24938
Labels
topic: plugins-PWA Issues related to PWA: the gatsby-plugin-offline and gatsby-plugin-manifest plugins type: bug An issue or pull request relating to a bug in Gatsby

Comments

@nibtime
Copy link
Contributor

nibtime commented Jun 11, 2020

Description

I observed, that the plugin has an npm dependency to idb-keyval and copies idb-keyval-iife.min.js from the node_modules folder into the public folder in onPostBuild:

const idbKeyvalFile = `idb-keyval-iife.min.js`
const idbKeyvalSource = require.resolve(`idb-keyval/dist/${idbKeyvalFile}`)
const idbKeyvalDest = `public/${idbKeyvalFile}`
fs.createReadStream(idbKeyvalSource).pipe(fs.createWriteStream(idbKeyvalDest))

The official caching recommendations are oblivious of that file and it would be cached with cache-control: public, max-age=31536000, immutable.

Furthermore, the plugin itself will cache the file with the CacheFirst strategy by default:

{
// Use cacheFirst since these don't need to be revalidated (same RegExp
// and same reason as above)
urlPattern: /(\.js$|\.css$|static\/)/,
handler: `CacheFirst`,
},

This is based on the assumption that all .css and .js files are fingerprinted, yet idb-keyval-iife.min.js is not.

If a future version of the plugin decides to use another version of idb-keyval this will become a problem since clients will not receive the update of the new version of idb-keyval because it is cached as immutable.

Idea for a fix

Read the version of idb-keyval from package.json in node_modules/idb-keyval and include it in the filename. Instead of calling importScripts with a literal file name in sw-append.js, it could be prepended here with the filename that includes the version:

const swAppend = fs
.readFileSync(`${__dirname}/sw-append.js`, `utf8`)
.replace(/%pathPrefix%/g, pathPrefix)
.replace(/%appFile%/g, appFile)

This solves the problem without the need to alter any existing caching configurations. I can send a PR if this is wanted.

Environment

gatsby info --clipboard
 System:
   OS: Linux 4.19 Debian GNU/Linux 10 (buster) 10 (buster)
   CPU: (8) x64 Intel(R) Core(TM) i7-6770HQ CPU @ 2.60GHz
   Shell: 5.7.1 - /usr/bin/zsh
 Binaries:
   Node: 10.20.1 - /tmp/yarn--1591893009228-0.6481271662017773/node
   Yarn: 1.22.4 - /tmp/yarn--1591893009228-0.6481271662017773/yarn
   npm: 6.14.4 - /usr/local/bin/npm
 Languages:
   Python: 2.7.16 - /usr/bin/python
 Browsers:
   Firefox: 68.8.0esr
yarn list | grep -E "(gatsby-plugin-offline|idb-keyval)" | grep -v deduped
│  ├─ gatsby-plugin-offline@^3.0.35
├─ [email protected]
│  ├─ idb-keyval@^3.2.0
├─ [email protected]
@nibtime nibtime added the type: bug An issue or pull request relating to a bug in Gatsby label Jun 11, 2020
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jun 11, 2020
@ascorbic ascorbic added topic: plugins-PWA Issues related to PWA: the gatsby-plugin-offline and gatsby-plugin-manifest plugins and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: plugins-PWA Issues related to PWA: the gatsby-plugin-offline and gatsby-plugin-manifest plugins type: bug An issue or pull request relating to a bug in Gatsby
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants