GoogleChrome / workbox

📦 Workbox: JavaScript libraries for Progressive Web Apps
https://developers.google.com/web/tools/workbox/
MIT License
12.35k stars 816 forks source link

Unexpected behaviour when manually constructing PrecacheController #2665

Closed JordanPawlett closed 6 months ago

JordanPawlett commented 3 years ago

Library Affected: workbox-precaching

Browser & Platform: Google Chrome latest

Issue or Feature Request Description: Using Workbox v5.1.4.

I have a use case where I want to fully utilise the precaching of my app shell, besides in the scenario where our server deems under certain circumstances that the user should be redirected. This redirection will occur on the base route / . This redirection will then be handled by the precacheController using the registered index.html asset given to precacheAndRoute, serving the user the cache index.html app shell rather than the browser redirecting to the page being served by a different server (same origin).

I have tried constructing my own precacheController and invoking install manually on it, inside the install listener. Passing install a plugins array containing a cacheWillUpdate plugin. Ideally preventing installation of the service worker if the redirect on / is present. Causing the user to be redirected correctly.

// issue lies here This approach appears not to be very consistent, depending on the order i construct things, i either never get a successful installation of the service worker on first load (on 200 where the index.html should be cached). Or an error is thrown when trying to grab index.html "c.handle is not a function" (minified workbox). I've added a default handler for it to fallback on, however it seems to be trying to call handle on the PrecacheController instance rather than its handler...

// Attempts to work around Furthermore, I have also tried registering a NetworkFirst strategy for just the index.html before the precacheAndRoute is called with the assets. This works great, however, i then lose the Navigation routing from createHandlerBoundToURL on longer urls and ones with queries, which is very important for our use-cases!

It seems like in Workbox v6 the PrecacheController has changed slightly to accept plugins as a parameter in its constructor. However i would not like to update as i'm using create-react-app for my webpack config.


import { clientsClaim } from 'workbox-core';
import { precacheAndRoute, PrecacheController } from 'workbox-precaching';
import { registerRoute, setDefaultHandler } from 'workbox-routing';

clientsClaim();

// precache all assets from the /build folder.
precacheAndRoute(self.__WB_MANIFEST);

const preCacheController = new PrecacheController();

const fileExtensionRegexp = new RegExp('/[^/?]+\\.[^/]+$');
// Handle navigation routes.
registerRoute(
  ({ request, url }) => {
    if (request.mode !== 'navigate') {
      return false;
    }

    if (url.pathname.startsWith('/_')) {
      return false;
    }

    if (url.pathname.match(fileExtensionRegexp)) {
      return false;
    }

    return true;
  },
  preCacheController.createHandlerBoundToURL(process.env.PUBLIC_URL + '/index.html')
);

setDefaultHandler(({ url, event, params }) => {
  return fetch(event.request);
});

self.addEventListener('install', (event) => {
  const plugins = [
    {
      cacheWillUpdate: ({ event, request, response }) => {
        if (response && response.status && response.status >= 300) {
          return false;
        }
        return true;
      }
    }
  ];

  event.waitUntil(
    preCacheController.install({ event, plugins })
  );
});
// more unrelated service worker code below.
jeffposnick commented 3 years ago

(CC: @philipwalton as he's done more recent investigation around PrecacheController's edge cases, in both v5 and v6.)

Do you want your service worker to be installed at all when there's a redirection performed?

If the redirection means that your service worker installation should fail, one approach would be to use the default precacheAndRoute() behavior, but add in an additional install handler at the top of your service worker file that performs the basic check around the redirect, and passes a rejected promise to event.waitUntil() when the service worker shouldn't be installed. This will cause installation to fail (regardless of what precacheAndRoute() would have done). The downside is that installation will be retried after each registration attempt, which could be "noisy" in terms of errors in the JS console, but that shouldn't affect the underlying functionality.

Something like:

import {precacheAndRoute} from 'workbox-precaching';

async function installCheck() {
  const response = await fetch('/');
  // See https://developer.mozilla.org/en-US/docs/Web/API/Response/redirected
  if (response.redirected) {
    // This will cause the promise returned by the async function to reject.
    throw new Error(`The request for '/' led to a redirected response.`);
  }
}

self.addEventListener('install', (event) => {
  // If installCheck() returns a rejected promise, service worker installation will fail.
  event.waitUntil(installCheck());
});

// If service worker installation failed, this will effectively be skipped.
precacheAndRoute(self.__WB_MANIFEST);
JordanPawlett commented 3 years ago

I do not want the service worker to successfully install if a redirect occurs, as this means the user should be taken to the login page, which should not handled be the service worker (it's served from server side static html separate from the app due to project specifics).

I'll give your suggestion a go!

What is the significance of adding a listener to the install event before the precacheAndRoute call? Is this due to the order the listeners are invoked? If this is the case, maybe we should consider adding listeners created and bound by the underlying API implementation to be pushed to the back, or only add the listeners once the users synchronous event queue is empty. The equivalent of using process.nextTick to add listeners. (although it's my understanding you don't have access to process in the service worker.)

edit: I've added that last bit, as typically when using an API the user shouldn't have to worry about the complexities of any specific events the underlying API may be registering. Due to Javascripts async nature a user should be able to register a listener anywhere in a synchronous file, only having to worry about the order of their own listeners. Hence the process.nextTick suggestion for internal events.

jeffposnick commented 3 years ago

A service worker's listeners listeners for a given event are executed sequentially, and synchronously, in the order in which they're registered. If your install handler executes first and causes installation to fail, you'd save the bandwidth that the install handler registered by precacheAndRoute() would normally consume to populate the precache. (Even if you listed precacheAndRoute() first, the cache would be populated and never used, so functionality wouldn't be affected.)

There are some restrictions in the service worker specification that mean you need to register all event listeners as part of the initial, top-level, execution of the service worker script—officially, you can't register additional listeners after that.

JordanPawlett commented 3 years ago

Thank you for clearing up the whole listener thing!

I've been using service-workers passively for a while. But it's only been in the past 2 days i've really deep dived into all their capabilities, particularly surrounding workbox.

JordanPawlett commented 3 years ago

Although I could make a fetch against an auth endpoint before installation, I've gone back to the plugin approach to test. I'm still not sure constructing your own PreCacheController or the plugin mechanism is functioning as expected.

On initial load, all requests return a 200, meaning the plugin model should allow the request to be cached and the installation successful. However, installation fails on the first precache entry Uncaught non-precached-url: non-precached-url :: [{"url":"./index.html"}]

import { clientsClaim } from 'workbox-core';
import { precacheAndRoute, PrecacheController } from 'workbox-precaching';
import { registerRoute } from 'workbox-routing';

clientsClaim();

const precacheController = new PrecacheController();
self.addEventListener('install', (event) => {
  const plugins = [
    {
      cacheWillUpdate: ({ event, request, response }) => {
        if (response && response.status && response.status >= 300) {
          return false;
        }
        return true;
      }
    }
  ];

  event.waitUntil(
    precacheController.install({ event, plugins })
  );
});

// precache all assets from the /build folder.
precacheAndRoute(self.__WB_MANIFEST);

const fileExtensionRegexp = new RegExp('/[^/?]+\\.[^/]+$');
// Handle navigation routes.
registerRoute(
  ({ request, url }) => {
    // If this isn't a navigation, skip.
    if (request.mode !== 'navigate') {
      return false;
    }

    // If this is a URL that starts with /_, skip. (most likely api call)
    if (url.pathname.startsWith('/_')) {
      return false;
    }

    // If this looks like a URL for a resource, because it contains // a file extension, skip.
    if (url.pathname.match(fileExtensionRegexp)) {
      return false;
    }

    // use this handler
    return true;
  },
  precacheController.createHandlerBoundToURL(process.env.PUBLIC_URL + '/index.html')
);
johnnyreilly commented 3 years ago

I don't know if this is the same issue but the non-precached-url caught my eye. My app has been throwing errors like this:

Uncaught (in promise) non-precached-url: non-precached-url :: [{"url":"/rudder//index.html"}]

Given a site.webmanifest of:

{
    "name": "Rudder",
    "short_name": "Rudder",
    "icons": [
        {
            "src": "/rudder/rudder-icon.png",
            "sizes": "512x512",
            "type": "image/png"
        }
    ],
    "start_url": "/rudder/index.html",
    "theme_color": "#4a148c",
    "background_color": "#000000",
    "display": "standalone"
}

It looks that somehow /rudder/index.html is gaining a second \ and becoming /rudder//index.html - not sure how this is happening. But I thought I'd share it here in case relevant. In my case the service worker is being generated using Create React Apps service worker support.

The built service-worker.js contains this tell-tale line: registerRoute(new e.NavigationRoute(e.createHandlerBoundToURL("/rudder//index.html") - again not sure if this the same issue or a different one.

update

I suspect this is a Create React App issue - still investigating

jeffposnick commented 3 years ago

@johnnyreilly, is this c-r-a v3 or v4?

johnnyreilly commented 3 years ago

C-R-A 3

Just had a look at 4 and it has a different approach. (Implemented by you I think 🤗)

https://github.com/facebook/create-react-app/pull/9205

I'm pondering upgrading to C-R-A 4 and seeing if it resolves this. Good shout?

jeffposnick commented 3 years ago

Yeah, I'd switch to c-r-a v4 at this point if you're having any issues with c-r-a v3's config. That should give you the flexibility to make any changes to the underlying service worker file to, e.g., account for something in your publicPath prefix (which sounds like what you might have been running into).

johnnyreilly commented 3 years ago

I'll give it a go - thanks!

tomayac commented 6 months ago

Hi there,

Workbox is moving to a new engineering team within Google. As part of this move, we're declaring a partial bug bankruptcy to allow the new team to start fresh. We realize this isn't optimal, but realistically, this is the only way we see it working. For transparency, here're the criteria we applied:

Thanks, and we hope for your understanding! The Workbox team