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

Unexpected behaviour when manually constructing PrecacheController #2665

Closed
JordanPawlett opened this issue Nov 5, 2020 · 11 comments
Closed
Labels
Needs More Info Waiting on additional information from the community. workbox-precaching

Comments

@JordanPawlett
Copy link

JordanPawlett commented Nov 5, 2020

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
Copy link
Contributor

(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);

@jeffposnick jeffposnick added Needs More Info Waiting on additional information from the community. workbox-precaching labels Nov 5, 2020
@JordanPawlett
Copy link
Author

JordanPawlett commented Nov 5, 2020

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
Copy link
Contributor

jeffposnick commented Nov 5, 2020

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
Copy link
Author

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
Copy link
Author

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
Copy link

johnnyreilly commented Nov 9, 2020

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
Copy link
Contributor

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

@johnnyreilly
Copy link

johnnyreilly commented Nov 9, 2020

C-R-A 3

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

facebook/create-react-app#9205

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

@jeffposnick
Copy link
Contributor

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
Copy link

I'll give it a go - thanks!

@tomayac
Copy link
Member

tomayac commented Apr 25, 2024

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

@tomayac tomayac closed this as completed Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs More Info Waiting on additional information from the community. workbox-precaching
Projects
None yet
Development

No branches or pull requests

4 participants