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

[Web] Don't force-reload the Service Worker #561

Merged
merged 9 commits into from
Jun 18, 2023

Conversation

adamziel
Copy link
Collaborator

@adamziel adamziel commented Jun 17, 2023

Description

Auto-reloading the service worker when it's updated thrashes any postMessage communication that's in progress at the moment of calling skipWaiting(). This causes Playground to hang at the "login" step in #559.

This PR removes the entire concept of detecting the service worker version and enforcing an update on mismatch.

The browsers handle a lot by default:

  • registration.update() method downloads the new service-worker.js file and compares it byte-by-byte with the existing one
  • The previous service worker won't die until all the browser tabs it serves are closed
  • The new service worker will automatically replace the previous one afterwards

The only problem remains deploying a website that is backwards–incompatible with the previous service worker.

Let's handle that case as follows:

  • Avoid changes that break the website before the service worker gets updated.
  • If they can't be avoided, let's ship them with a service worker that forces a one-off skipWaiting() and reloads all served browser tabs.

cc @ellatrix @dmsnell

@adamziel adamziel force-pushed the fix-progress-bar-stuck-at-login-issue branch from 536ee3f to 746f215 Compare June 17, 2023 16:16
@adamziel
Copy link
Collaborator Author

adamziel commented Jun 17, 2023

This PR solves one issue but creates another – despite the window.top.location.reload() call the service worker is only actually reloaded on a second page refresh. Once we figure that out, we'll be good here.

@dmsnell
Copy link
Member

dmsnell commented Jun 18, 2023

I'm confused here. Is the problem that the service worker upgrades itself to newer copies of the service worker code, but we initiate that?

Is there a way to reject updates? Why apply the update in the middle of a session when we could choose to only update at app bootup? Or when there are no messages going on (consider, for example, triggers at a new session at at some number of minutes since the last activity from a session)

@adamziel
Copy link
Collaborator Author

adamziel commented Jun 18, 2023

@dmsnell I still find service workers confusing.

  • Once it gets registered, it lives in the browser forever. If you visit https://playground.wordpress.net/ today and return in a month, you'll still have the previous service worker. However, if there was a deploy in the meantime, there's a chance the new website is incompatible with the old service worker – which is why we want to update it.
  • At the same time, if someone is on https://playground.wordpress.net/ at the exact time we deploy a new service worker, I'd prefer them to keep using the previous version instead of force-reloading their browser tab. This means any new playground tab must load the old JS, CSS etc files.
  • Also, a single service worker serves all open tabs. It won't update until you close all your Playground tabs, unless the service worker itself forces it by calling self.skipWaiting()

I'm starting to think the only bulletproof solution here might be doing a PWA. This way we could auto-update on return, but keep using the old version if you open a new playground in an active browsing session.

dmsnell and others added 5 commits June 18, 2023 17:12
When mounting local directories in #548, problems appeared in development
builds because the dev environemnt runs a separate server for the website
that wraps the iframe and the for the assets inside the iframe, the "remote."

In this patch we're adjusting the config for the outer website server so that
we can proxy requests for the internal frame through the same origin. This is
done by adding a prefix to the URL for the dev assets on the outer frame, which
then is used in a proxy rule to decide what to route.

This means that OPFS handles can be shared between the two sides of the iframe,
which normally happens in production since they both come from the same origin.
So far I found three problems:

* Message handler in broadcastMessageAwaitReply() seems to get bound
  after the response is received by the service worker. Fix: bind it
  before dispatching the message
* Setting the iframe source in bindMessageHandler() triggers a request
  message that is handled but the response to that message is not
  processed in the service worker. Not sure why.
* Auto-reloading the service worker when it's updated thrashes any
  postMessage communication that's in progress at the moment of calling
  update() and skipWaiting(). Ideal UX would be to figure out that flow,
  but easy fix is to reload the page after invalidating the stale
  service worker
@adamziel adamziel force-pushed the fix-progress-bar-stuck-at-login-issue branch from 8b119f5 to bf82ce6 Compare June 18, 2023 15:12
@adamziel
Copy link
Collaborator Author

adamziel commented Jun 18, 2023

I removed the entire concept of detecting the service worker version and enforcing an update on mismatch.

I just realized the browsers give us a lot by default:

  • The update() method downloads the new service-worker.js file and compares it byte-by-byte with the existing one
  • The previous service worker won't die until all the browser tabs it serves are closed
  • The new service worker will automatically replace the previous one afterwards

The only case this doesn't cover is "the newly deployed site is backwards–incompatible and requires the new worker".

Let's handle that case as follows:

  • Avoid changes that break the website before the service worker gets updated.
  • If they can't be avoided, let's ship them with a service worker that forces a one-off skipWaiting() and reloads all served browser tabs.

The latter would mean reloading work-in-progress tabs for folks who:

  • Use Playground as that change is rolled out
  • Open a new Playground tab while the existing tabs are still active

Since that could be potentially destructive, that new tab could ask for permissions, e.g.

New Playground version was just released. You can continue using your current browser tabs, but if you need to open a new tab then Playground will reload ALL your currently open tabs. This will destroy your work in progress on all your temporary sites but won't affect your permanent sites. Would you like to proceed?

@adamziel adamziel marked this pull request as ready for review June 18, 2023 16:51
@adamziel adamziel changed the title Fix progress bar stuck at login issue [Web] Don't force-reload the Service Worker Jun 18, 2023
@adamziel adamziel added [Type] Bug An existing feature does not function as intended [Type] Enhancement New feature or request [Aspect] Browser and removed [Type] Enhancement New feature or request labels Jun 18, 2023
@adamziel adamziel force-pushed the fix-progress-bar-stuck-at-login-issue branch from f1e3cbf to 10e6625 Compare June 18, 2023 17:02
@adamziel adamziel merged commit f78ff38 into trunk Jun 18, 2023
@adamziel adamziel deleted the fix-progress-bar-stuck-at-login-issue branch June 18, 2023 17:23
adamziel added a commit that referenced this pull request Jun 18, 2023
Removes the entire concept of detecting the service worker version and enforcing an update on mismatch.

Auto-reloading the service worker when it's updated thrashes any `postMessage` communication that's in progress at the moment of calling `skipWaiting()`. This causes Playground to hang at the "login" step in #559.

However, the browsers handle a lot by default:

* `registration.update()` method downloads the new service-worker.js file and compares it byte-by-byte with the existing one
* The previous service worker won't die until all the browser tabs it serves are closed
* The new service worker will automatically replace the previous one afterwards

The only problem remains deploying a website that is backwards–incompatible with the previous service worker. This is tracked separately in #566

Co-authored-by: Dennis Snell <dennis.snell@automattic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Aspect] Browser [Aspect] Service Worker [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants