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

Make onUpdate callback fire when a new service worker is intalled and waiting #5491

Closed

Conversation

arackaf
Copy link

@arackaf arackaf commented Oct 20, 2018

Simple change to have the onUpdate callback fire when a newly installed service worker is waiting. This ensures the notification will not just come the first time the sw is updated, but rather each refresh until it takes over.

The modified code was run in my local project, and the onUpdate did continue to show on each refresh until all tabs were closed, and the new sw took over.

@Timer
Copy link
Contributor

Timer commented Oct 20, 2018

/cc @jeffposnick

@dominicarrojado
Copy link

How to do force the update without closing all tabs? How to implement skipWaiting with CRA?

@arackaf
Copy link
Author

arackaf commented Oct 20, 2018

That has to be done client side. skipWaiting would be called in the callback you provide for onUpdate, which would then post a message that client side code would have to receive, and then manually refresh.

@dominicarrojado
Copy link

But the event listener for the message to skipWaiting should be on the service worker file which is only available after build. Is there a way without touching the build files?

@arackaf
Copy link
Author

arackaf commented Oct 20, 2018

To automatically have service workers skipWaiting, you’d need to override the sw configuration using something like customize-cra

@dominicarrojado
Copy link

@arackaf Thank you, that's exactly what I need.

@jeffposnick
Copy link
Contributor

jeffposnick commented Oct 22, 2018

I chatted with @arackaf about this logic and it looks 👍

Thanks for putting the PR together!

// content until all client tabs are closed.
console.log(
'New content is available and will be used when all ' +
'tabs for this page are closed. See http://bit.ly/CRA-PWA.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Neat PR!

This bit.ly link needs to be pointed to https://facebook.github.io/create-react-app/docs/making-a-progressive-web-app now. I created http://bit.ly/CRA2-PWA but not sure if @gaearon wants to do it from his own account or something similar :)

Copy link
Contributor

Choose a reason for hiding this comment

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

There are some issues with updating these links as these files (with the original links) are present in existing projects. More details here: #5536

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, I see. Thanks.

Copy link
Author

Choose a reason for hiding this comment

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

done!

@arackaf
Copy link
Author

arackaf commented Nov 7, 2018

@jeffposnick - just tweaked things to pass the updated sw to the onUpdate callback, so client code can postMessage to it.

Anything blocking this from getting merged?

@tyler-dunkel
Copy link

@arackaf regarding your statement about automatically skipWaiting for SW.... does this mean to have the behavior of: "code has changed, have user refresh current tab for new content" would need the customization of c-r-a? I.E. does that mean that C-R-A only supports updating content from a SW by closing the tab and reopening the app in a new tab?

@arackaf
Copy link
Author

arackaf commented Nov 13, 2018

“closing and reopening the tab”

Yeah, by default that’s what the user has to do to get the new sw. Once this pr is merged you’ll be able to do something like this

https://twitter.com/adamrackis/status/1060036913921449984?s=21

@Timer
Copy link
Contributor

Timer commented Nov 13, 2018

Can you update the TS version too, please?

@arackaf
Copy link
Author

arackaf commented Nov 13, 2018

@Timer sorry? There's a TS version of this same file? Where?

@tyler-dunkel
Copy link

tyler-dunkel commented Nov 13, 2018

@arackaf Thanks for the quick answer! one other question if you dont mind: what does this mean in the context of installed apps on mobile homescreens?
i.e. if an android user has added the app to homescreen, do they need to kill the running process to get the updated content?

Also, regarding the tweet you shared, I see that part of that solution involves updating the service-worker file itself to add the message event handler. So this would require using something to customize C-R-A's generated service-worker file?

@inssein
Copy link

inssein commented Nov 21, 2018

@arackaf this PR only makes sense once you can customize you service worker to skip waiting. I saw your tweet, but don't understand where the code in the third screenshot is placed.

@NShahri
Copy link
Contributor

NShahri commented Nov 22, 2018

I am trying to describe 2 scenarios which can be used in PWA app to show message, reload the app UI
and install new service worker:

  • CASE 1: using skipWaiting config of WorkBox of WebPack plugin
    When user reopened the page or after update method has been called, state of new service worker would change to installed first and activated afterwards, but the app UI would be old one. Showing a message which tells a user to reload the page would be useful.

  • CASE 2: no skipWaiting config for WorkBox
    Based on Advanced Recipes, we can trigger skipWaiting and reload the page when service worker gets activated (not sure how it is possible with WorkBox Webpack Plugin), but in this method we can minimise time between updating service worker and updating UI.

Requirements:

  • CASE 1 requirements:
    • onUpdateActivated event/callback
  • CASE 2 requirements:
    • onUpdateAvailable event/callback
    • onUpdateActivated event/callback

onUpdateAvailable/onUpdate callback
It is exactly current onUpdate implementation.

onUpdateActivated/onActivated callback
It can be implemented in one of these ways:

navigator.serviceWorker.addEventListener('controllerchange’, ()=>{})

OR

registration.installing.onstatechange = () => {
    if (registration.installing.state === ‘activated’) {}
}

@arackaf
Copy link
Author

arackaf commented Nov 24, 2018

@inssein screenshot #3 is code that you manually wire into the service worker via importScripts, via workbox configuration

https://github.com/arackaf/booklist/blob/master/react-redux/webpack.config.js#L116

@arackaf
Copy link
Author

arackaf commented Nov 24, 2018

@Timer sorry for the delay - how's that look?

@inssein
Copy link

inssein commented Nov 24, 2018

@inssein screenshot #3 is code that you manually wire into the service worker via importScripts, via workbox configuration

https://github.com/arackaf/booklist/blob/master/react-redux/webpack.config.js#L116

You can't do that without ejecting though, right?

@arackaf
Copy link
Author

arackaf commented Nov 24, 2018

@inssein - ah, sorry, no need to eject - just a lil tweak is all you need :)

https://github.com/arackaf/customize-cra#adjustworkboxfn

@samccone
Copy link
Contributor

samccone commented Jan 4, 2019

@gaearon Hey Dan, anything I can do to help move this along? This is a major win to those of us out there that are using CRA + SW integration.

Thanks so much for your time and please do let me know what I can do to help.

@stale
Copy link

stale bot commented Feb 7, 2019

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Feb 7, 2019
@danielkcz
Copy link

Well, this is getting kinda sad. This thing is working quite nicely, I have it in a production of two apps. Not sure why it's being held back so badly.

@stale stale bot removed the stale label Feb 7, 2019
@dominicarrojado
Copy link

Yeah, same here as well. I also used this for my apps.

@stale
Copy link

stale bot commented Mar 9, 2019

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Mar 9, 2019
@danielkcz
Copy link

Here we go again :) Stale bot, go away!

@stale
Copy link

stale bot commented Apr 23, 2019

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Apr 23, 2019
@arackaf
Copy link
Author

arackaf commented Apr 23, 2019

Any reason this was never merged?

@stale stale bot removed the stale label Apr 23, 2019
@iansu
Copy link
Contributor

iansu commented Apr 24, 2019

@arackaf No, I don't think there was a reason. It just fell through the cracks while we were working on 3.0. We'll take a closer look at it and see if we can get it into an upcoming release.

@iansu iansu self-assigned this Apr 24, 2019
@arackaf
Copy link
Author

arackaf commented Apr 24, 2019

Sweet, thanks! Sorry if my comment came off as snarky - I just needed something so the bot wouldn't close, and that's what my brain tossed out :)

@iansu
Copy link
Contributor

iansu commented Apr 24, 2019

That's actually one of the nicer responses to the stale bot that I've seen. 😀

@FezVrasta
Copy link
Contributor

Should you merge it?

@andriijas
Copy link
Contributor

@arackaf thanks for your patience. could you rebase your branch on top of the latest changes?

thanks

@arackaf
Copy link
Author

arackaf commented Jan 31, 2020

Not really. It's been years since I even looked at this. I'd recommend just merging and pushing.

@andriijas
Copy link
Contributor

That was my idea. I had some time over today and have been working through long running PRs. Given this was already approved I wanted to get it in however there are conflicting files.

I understand your frustration. Just let us know if you want to give it one more time or if we should close this or if there is someone else who can continue.

Sorry for any inconvinience

@arackaf
Copy link
Author

arackaf commented Jan 31, 2020

Yeah it's been years since I even looked at this project. I wouldn't really feel comfortable trying to resolve conflicts, nor am I in a position to test the resuls easily.

@andriijas andriijas closed this Jan 31, 2020
@lock lock bot locked and limited conversation to collaborators Feb 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.