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): Allow appending custom scripts to sw.js #11626

Merged
merged 21 commits into from
Aug 30, 2019
Merged

feat(gatsby-plugin-offline): Allow appending custom scripts to sw.js #11626

merged 21 commits into from
Aug 30, 2019

Conversation

vtenfys
Copy link
Contributor

@vtenfys vtenfys commented Feb 7, 2019

No description provided.

@vtenfys
Copy link
Contributor Author

vtenfys commented Feb 19, 2019

@pieh should be ready to merge now!


if (pluginOptions.injectScript) {
const userAppend = fs.readFileSync(pluginOptions.injectScript, `utf8`)
fs.appendFileSync(`public/sw.js`, `\n` + userAppend)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of wrapping this in a try catch here and printing a friendly error if the file doesn't exist? Definitely not a blocker for this PR though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I'll implement this change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, just done this!

@sidharthachatterjee
Copy link
Contributor

Left a few comments but overall looks great, @davidbailey00! 👍

@pieh pieh added the breaking change If implemented, this proposed work would break functionality for older versions of Gatsby label Mar 18, 2019
@DSchau DSchau added the status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response label Mar 26, 2019
@pieh pieh self-assigned this May 18, 2019
@pieh
Copy link
Contributor

pieh commented May 20, 2019

I was just checking some docs - and was wondering if we could just document how to use importScripts workbox config options instead of implementing our custom one?

@pieh pieh added status: awaiting author response Additional information has been requested from the author and removed status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response labels May 21, 2019
@m-allanson
Copy link
Contributor

@davidbailey00 if it's possible to use workbox's importScripts, let's close this PR and create a separate issue.

@KyleAMathews
Copy link
Contributor

It was introduced in Workbox 4.3

@KyleAMathews
Copy link
Contributor

So we can't use it since we're on v3 still.

@m-allanson
Copy link
Contributor

Would it be worth updating to workbox 4 instead of doing this?

@vtenfys
Copy link
Contributor Author

vtenfys commented May 27, 2019

Hmm we could use the Workbox option - it wouldn't just be a one-stop operation though, since we would need to insert the content hash into the filename for any custom scripts this way and then modify the filenames in the config to match, so the first version is not cached forever. The approach taken in this PR instead inserts the custom code into the sw.js file itself. I think either way would work fine, so perhaps it would be best to upgrade Workbox and use the new option after all rather than an API breaking change?

@pieh
Copy link
Contributor

pieh commented May 27, 2019

Hmm we could use the Workbox option - it wouldn't just be a one-stop operation though, since we would need to insert the content hash into the filename for any custom scripts this way and then modify the filenames in the config to match, so the first version is not cached forever

Maybe we could do this on user behalf if importScripts was used (i.e. rename file to attach content hash and send renamed filename to workbox)? Not sure if this wouldn't be confusing tho. Or even just completely took over importScripts option and do what is currently done in this PR and strip that field from options passed to workbox?

@m-allanson
Copy link
Contributor

Moving this out of "In Progress" for now. We can circle back to it.

Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you so much for wrapping this up @davidbailey00 🥇

packages/gatsby-plugin-offline/README.md Outdated Show resolved Hide resolved
@sidharthachatterjee sidharthachatterjee removed the status: awaiting author response Additional information has been requested from the author label Aug 16, 2019
Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@sidharthachatterjee
Copy link
Contributor

Need to fix these

The 'dontCacheBustUrlsMatching' option has been renamed to 'dontCacheBustURLsMatching'. Please update your config. 'dontCacheBustUrlsMatching' is now deprecated and will be removed in a future release of Workbox.
The 'modifyUrlPrefix' option has been renamed to 'modifyURLPrefix'. Please update your config. 'modifyUrlPrefix' is now deprecated and will be removed in a future release of Workbox.
Specifying 'cacheFirst'' in a 'runtimeCaching[].handler' option is deprecated. Please update your config to use 'CacheFirst' instead. In v4 Workbox strategies are now classes instead of functions.
Specifying 'networkFirst'' in a 'runtimeCaching[].handler' option is deprecated. Please update your config to use 'NetworkFirst' instead. In v4 Workbox strategies are now classes instead of functions.
Specifying 'staleWhileRevalidate'' in a 'runtimeCaching[].handler' option is deprecated. Please update your config to use 'StaleWhileRevalidate' instead. In v4 Workbox strategies are now classes instead of functions.
Specifying 'staleWhileRevalidate'' in a 'runtimeCaching[].handler' option is deprecated. Please update your config to use 'StaleWhileRevalidate' instead. In v4 Workbox strategies are now classes instead of functions.

@sidharthachatterjee sidharthachatterjee added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Aug 16, 2019
@sidharthachatterjee
Copy link
Contributor

sidharthachatterjee commented Aug 16, 2019

This is good to go and we will publish it on Monday!

@sidharthachatterjee sidharthachatterjee removed the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Aug 20, 2019
@vtenfys vtenfys changed the title feat(gatsby-plugin-offline): Allow injection into sw.js feat(gatsby-plugin-offline): Allow appending custom scripts to sw.js Aug 22, 2019
@sidharthachatterjee
Copy link
Contributor

Running this on org and posting a build url here in a bit

@sidharthachatterjee
Copy link
Contributor

Looks like this works great on https://gatsby-org-offline3-test.netlify.com/

Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! 💃

@sidharthachatterjee sidharthachatterjee merged commit 275344e into gatsbyjs:master Aug 30, 2019
@vtenfys
Copy link
Contributor Author

vtenfys commented Aug 30, 2019

Published in [email protected] 🎉

waltercruz pushed a commit to waltercruz/gatsby that referenced this pull request Sep 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change If implemented, this proposed work would break functionality for older versions of Gatsby
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants