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 routes are not compatible with other packages #1857

Closed
philipwalton opened this issue Jan 23, 2019 · 3 comments · Fixed by #2639
Closed

workbox-precaching routes are not compatible with other packages #1857

philipwalton opened this issue Jan 23, 2019 · 3 comments · Fixed by #2639
Assignees
Labels
Discuss An open question, where input from the community would be appreciated. Feature Request

Comments

@philipwalton
Copy link
Member

philipwalton commented Jan 23, 2019

Library Affected:
workbox-precaching

The addRoute() (and by extension precacheAndRoute()) method in workbox-precaching adds its own fetch event listener + cache-first response logic rather than depending on the Router and Route modules from workbox-routing and the CacheFirst module from workbox-strategies.

This is not ideal for a few reasons:

  • Code is duplicated and needs to be tested twice
  • Logging for precached routes is different from logging for all other routes
  • Precache route logic (the configuration options that addRoute() and precacheAndRoute() offer) is not available to runtime routes.
  • Precache route handlers are not exposed to the developer, so they can't be reused with workbox-streams code (instead, developers have to create their own CacheFirst strategy instances anyway).

I'm sure there's historical reasons for (from the sw-precache/sw-toolbox merge), but I think we should explore whether it would be possible to consolidate these features in the future.

IMO, ideally, workbox-precaching would depend on workbox-routing and workbox-strategies, and the configuration options for customizing preaching routes could be added to the workbox-routing package and thus available to all routes.

@philipwalton philipwalton added Discuss An open question, where input from the community would be appreciated. Feature Request labels Jan 23, 2019
@philipwalton philipwalton self-assigned this Jan 23, 2019
@jeffposnick
Copy link
Contributor

Historically, there were members of the core team who felt that minimizing cross-module dependencies was a priority, so that using workbox-precaching wouldn't automatically incur the overheard of the module size of workbox-routing and workbox-strategies.

I am... less concerned about that, and in particular, if we assume that more folks are going to create custom Workbox bundles in the future, evaluating costs based on the full bundle size of a given module isn't necessarily as meaningful.

@jeffposnick
Copy link
Contributor

jeffposnick commented Jan 24, 2019

For some more historical context, the v2 codebase had all of this logic inside of the common workbox-sw module, kind of mixed together:

// Create a Router instance that's used by the `Route` for precached assets.
// See https://github.com/GoogleChrome/workbox/issues/839
this._precacheRouter = new Router(
this._revisionedCacheManager.getCacheName(),
);
this._router = new Router(
this._revisionedCacheManager.getCacheName(),
);
if (handleFetch) {
// Give precedence to the _precacheRouter by registering its `fetch`
// handler first.
this._precacheRouter.addFetchListener();
this._router.addFetchListener();
}

(I'm not arguing that we should return to that catch-all workbox-sw module; just sharing it for context.)

With v3, we started separating things out into self-contained modules with minimal overlap or cross-module code sharing.

@divyeshsachan
Copy link

I do vote for @philipwalton for this, since it'll expose all options of workbox-strategies to precache eventually resolving issue #1760 and add great flexibility. By default it should depend on CacheFirst / CacheFirst strategy.
After this developers can use other strategies for precache like staleWhileRevalidate, etc, refer https://stackoverflow.com/questions/54651722/how-can-i-set-a-default-setcatchhandler-with-google-workbox-v4 and #1897.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discuss An open question, where input from the community would be appreciated. Feature Request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants