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

workbox-precaching should have precedence over workbox-routing #2402

Closed
jeffposnick opened this issue Mar 12, 2020 · 2 comments · Fixed by #2639
Closed

workbox-precaching should have precedence over workbox-routing #2402

jeffposnick opened this issue Mar 12, 2020 · 2 comments · Fixed by #2639
Labels
Breaking Change Denotes a "major" semver change. Discuss An open question, where input from the community would be appreciated. workbox-precaching

Comments

@jeffposnick
Copy link
Contributor

Library Affected:
workbox-precaching

We currently delay registering the fetch handler for workbox-precaching until the point where precacheAndRoute() is called:

function precacheAndRoute(entries: Array<PrecacheEntry|string>, options?: FetchListenerOptions) {
precache(entries);
addRoute(options);
}

We should consider registering this earlier, as a side effect of importing workbox-precaching. If the user doesn't end up calling precacheAndRoute(), then the fetch handler will effectively be a no-op. Alternatively, we should consider finding a way to share a common fetch handler between workbox-precaching and workbox-routing.

This works around an unintuitive issues where including any reference to workbox-routing's registerRoute() prior to calling precacheAndRoute() would cause all runtime caching routes that are registered to take precedence of workbox-precaching, even if those runtime caching routes are listed after precacheAndRoute().

The downside is that if a developers wants a runtime caching route registered with workbox-routing to take precedence over precaching, they won't be able to accomplish that. I'd argue that they shouldn't precache a URL that they plan on serving with runtime caching, but maybe there are some valid use cases I'm not thinking of.

This recently caused some confusion for the https://web.dev/ folks. CC: @samthor @philipwalton

@jeffposnick jeffposnick added Discuss An open question, where input from the community would be appreciated. Breaking Change Denotes a "major" semver change. workbox-precaching labels Mar 12, 2020
@samthor
Copy link
Contributor

samthor commented Mar 18, 2020

Our use case is basically:

  • images live under /images/ and include both core site images as well as profile pictures etc
  • we only want to precache the things that appear on the home page
  • but we want to runtime cache any extended images that the user happens to load.

To be fair, we could be better about delineating where we put images. Fundamentally it just feels confusing that one handler can "win"—as an ignorant user (sorry) I would expect all handlers run by Workbox to be enumerated and attempted until a match was found, regardless of how we add them.

@jeffposnick
Copy link
Contributor Author

One of @philipwalton's longer-term projects is reconciling some of the one-off code in workbox-precaching that basically reimplements other parts of Workbox so that it actually depends on the actual Workbox classes instead. I think that would help with this particular class of problem, where workbox-precaching installs its own fetch handler instead of using the one from workbox-routing.

(The background here is that we previously focused on minimizing cross-package dependencies, back when you'd have to load all of workbox-routing if you used a little piece of it in workbox-precaching. Now that we are using modern bundling techniques, importing individual classes within a package incurs much less of a code size cost.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Denotes a "major" semver change. Discuss An open question, where input from the community would be appreciated. workbox-precaching
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants