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): reload when missing resources and SW was updated + add "onServiceWorkerUpdateReady" API #10432

Merged
merged 18 commits into from
Jan 28, 2019
Merged

Conversation

vtenfys
Copy link
Contributor

@vtenfys vtenfys commented Dec 12, 2018

Fixes #10036

@vtenfys vtenfys requested a review from a team as a code owner December 12, 2018 10:32
@vtenfys vtenfys requested a review from a team December 12, 2018 10:32
Copy link
Contributor

@shannonbux shannonbux 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 from a docs perspective. This can get merged after a technical review from anyone else on the OSS team!

@vtenfys
Copy link
Contributor Author

vtenfys commented Dec 13, 2018

@pieh (summary of what I said on Slack): the current API onServiceWorkerUpdateFound runs even for initial install - see register-service-worker.js. I think it could be beneficial to have an API which users can employ e.g. to show a prompt only when an update is found, but not for initial install.

@vtenfys
Copy link
Contributor Author

vtenfys commented Dec 13, 2018

@pieh this site https://www.mahmago.com/ is a good example of why we need a separate API - upon first visit it says "this site has updated"

@vtenfys vtenfys requested a review from a team December 15, 2018 10:35
@vtenfys vtenfys requested a review from a team as a code owner December 15, 2018 10:35

return cachedResponse
})
return caches.match(offlineShell, { cacheName })
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's safe to remove that yet. This handles stuff before runtime actually run, so we can't really rely that any changes we make in runtime are actually in effect (not to mention that people can use "missmatched" versions of gatsby and offline plugin in this could "brick" their sites still in those still not figured out scenarios)

makenowjust added a commit to makenowjust/commlog that referenced this pull request Jan 5, 2019
`onServiceWorkerUpdateFound`を使うようにドキュメントに書かれていた。

> ```javascript
> exports.onServiceWorkerUpdateFound = () => {
>   const answer = window.confirm(
>     `This application has been updated. ` +
>       `Reload to display the latest version?`
>   )
>
>   if (answer === true) {
>     window.location.reload()
>   }
> }
> ```
>
> https://www.gatsbyjs.org/docs/add-offline-support-with-a-service-worker/

が、これだとページを初めて開いたときにもこの表示が出てしまうので、`onServiceWorkerUpdateReady`というのを追加しようというPull Requestがある。

gatsbyjs/gatsby#10432

これがマージされたらこっちに切り替えよう。
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

LGTM but I haven't tested it myself but it seems to be solid.

I'll let @pieh to do a final review

@@ -34,8 +35,10 @@
},
"repository": "https://github.com/gatsbyjs/gatsby/tree/master/packages/gatsby-plugin-offline",
"scripts": {
"build": "babel src --out-dir . --ignore **/__tests__",
"build": "npm run build:src && npm run build:sw-append",
"build:src": "babel src --out-dir . --ignore **/__tests__",
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to ignore sw-append.js here? for builds it probably doesn't matter - it will be overwritten by cpx in next step, but while watching, babel will try to compile it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yep, that's a good point - need to fix that

pieh and others added 3 commits January 9, 2019 17:09
@vtenfys vtenfys changed the title refactor(gatsby-plugin-offline): only reload if necessary; introduce new "ready" API refactor(gatsby-plugin-offline): improve SW update logic + introduce new "ready" API Jan 25, 2019
@pieh
Copy link
Contributor

pieh commented Jan 28, 2019

Was testing this over the weekend and it seems like nice improvement - especially part that reloads page if there are missing resources and service worker did update
screenshot 2019-01-28 at 13 20 04

@pieh
Copy link
Contributor

pieh commented Jan 28, 2019

Just need to bump peerDependency in offline plugin as we do changes in both gatsby and offline plugin here, but I will handle that

@pieh pieh changed the title refactor(gatsby-plugin-offline): improve SW update logic + introduce new "ready" API feat(gatsby-plugin-offline): reload when missing resources and SW was updated + add "onServiceWorkerUpdateReady" API Jan 28, 2019
@pieh pieh merged commit 4a01c6d into gatsbyjs:master Jan 28, 2019
@sgnl
Copy link

sgnl commented Feb 27, 2019

Has anyone experieced this "missing recourses" error on a gatsby site without PWA or using gatsby-plugin-offline?

@vtenfys
Copy link
Contributor Author

vtenfys commented Mar 9, 2019

@sgnl Please could you open an issue describing this behaviour if you're still having it? Thanks 🙂 Just a heads up, sometimes it can be caused by adblockers so if you're using one, try disabling it and see if you get the same issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants