Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Deprecate workbox-routing's registerNavigationRoute() #2095

Closed
jeffposnick opened this issue Jul 8, 2019 · 7 comments · Fixed by #2143
Closed

Deprecate workbox-routing's registerNavigationRoute() #2095

jeffposnick opened this issue Jul 8, 2019 · 7 comments · Fixed by #2143
Labels
Breaking Change Denotes a "major" semver change. Discuss An open question, where input from the community would be appreciated. workbox-precaching workbox-routing
Milestone

Comments

@jeffposnick
Copy link
Contributor

workbox.routing.registerNavigationRoute() is a recurring source of confusion for developers (c.f. #1988 recently, as well as this archive of past issues in general).

workbox.routing.registerNavigationRoute() appears to be a general-purpose method of registering a route that matches navigation requests, when in fact, it's implemented as a way of bridging the gap between workbox-routing and workbox-precaching, making it easier to handle navigations using precached URLs.

Not everyone wants to handle navigations with precached URLs, though.

I think in v5, we might be able to get away with moving most of the logic in registerNavigationRoute() from workbox-routing to workbox-precaching, and have it just return logic that could be used as a handler callback. This handler callback could then be used when constructing a route.

If we moved this logic to workbox-precaching we could also avoid having to explicitly call workbox.precaching.getCacheKeyForURL(), which has been another source of confusion in Workbox v4.

// v4:
workbox.routing.registerNavigationRoute(
  workbox.precaching.getCacheKeyForURL('/app-shell.html')
);

// v5:
const handler = workbox.precaching.createHandlerForURL('/app-shell.html');
const navigationRoute = new workbox.routing.NavigationRoute(handler);
workbox.routing.regsiterRoute(navigationRoute);

CC: @philipwalton who was looking into the feasibility of using workbox-strategies under the hood for the handler logic in workbox-precaching, as this proposed change might impact that plan.

@jeffposnick jeffposnick added Discuss An open question, where input from the community would be appreciated. Breaking Change Denotes a "major" semver change. workbox-precaching workbox-routing labels Jul 8, 2019
@jeffposnick jeffposnick added this to the v5 milestone Jul 8, 2019
@madmoizo
Copy link

madmoizo commented Jul 10, 2019

If we moved this logic to workbox-precaching we could also avoid having to explicitly call workbox.precaching.getCacheKeyForURL(), which has been another source of confusion in Workbox v4.

In which way

workbox.routing.registerNavigationRoute(
  workbox.precaching.getCacheKeyForURL('/app-shell.html')
)

is more confusing than

workbox.routing.registerRoute(
  new workbox.routing.NavigationRoute(
    workbox.precaching.createHandlerForURL('/app-shell.html')
  )
)

?

I'm already using it
image

But it'is a basic use-case and as a user I just want an easy way to do it. Workbox could provide a sugar method with the URL as a param which abstract this common boilerplate.

@jeffposnick
Copy link
Contributor Author

The main source of confusion has to do with:

workbox.routing.registerNavigationRoute() appears to be a general-purpose method of registering a route that matches navigation requests, when in fact, it's implemented as a way of bridging the gap between workbox-routing and workbox-precaching, making it easier to handle navigations using precached URLs.

workbox-routing includes a class that has implementation details that are pretty specific to workbox-precaching, and it would be cleaner/less confusing to put those details inside of workbox-precaching. That's my thinking, at least—if folks disagree, we can reconsider.

@madmoizo
Copy link

madmoizo commented Jul 10, 2019

@jeffposnick I don't question the "where things should be" part. Just the dev experience part.

If I could do

import { routeToShell } from 'workbox-routing/routeToShell.mjs' // or workbox-precaching/

routeToShell('app-shell.html')

instead of

import { registerRoute } from 'workbox-routing/registerRoute.mjs'
import { NavigationRoute } from 'workbox-routing/navigationRoute.mjs'
import { createHandlerForURL } from 'workbox-precaching/createHandlerForURL.mjs'

registerRoute(
  new NavigationRoute(
    createHandlerForURL('/app-shell.html')
  )
)

I'll be happy !

@philipwalton
Copy link
Member

FWIW I'm a big fan of deprecating registerNavigationRoute(), as I know I've been one of the people confused by the name in the past. I also agree the right place for the new logic is in the workbox-precaching package.

I also think something like precaching.createHandlerForURL() is a good low-level primitive to make more complex strategy/plugin/route matching options possible.

If it's always going to only match navigation routes, do you think it's worth calling it createNavigationHandlerForURL()instead? Another possible name iscreateNavigationHandlerForPath()`.

@jeffposnick
Copy link
Contributor Author

jeffposnick commented Jul 25, 2019

If it's always going to only match navigation routes, do you think it's worth calling it createNavigationHandlerForURL() instead? Another possible name is createNavigationHandlerForPath().

My thinking is that the new method wouldn't do anything related to matching. It would just be a method in workbox-precaching that returned something which followed the handlerCallback interface.

You'd still need to combine it with something that took care of the matchCallback side of things, like wrap it in a NavigationRoute (if you wanted it to only match navigations):

const handler = workbox.precaching.createHandlerForURL('/app-shell.html');
const navigationRoute = new workbox.routing.NavigationRoute(handler, {
  whitelist: [...],
  blacklist: [...],
});
workbox.routing.registerRoute(navigationRoute);

The nuance here is that folks often need a way to include/exclude a subset of navigations when following the App Shell pattern, and the NavigationRoute class provides that matching capability in a standard fashion.

@philipwalton
Copy link
Member

My thinking is that the new method wouldn't do anything related to matching. It would just be a method in workbox-precaching that returned something which followed the handlerCallback interface.

You'd still need to combine it with something that took care of the matchCallback side of things, like wrap it in a NavigationRoute (if you wanted it to only match navigations):

Ahh, right, I overlooked that part. createHandlerForURL() SGTM then.

@hanjeahwan
Copy link

workbox.routing.registerNavigationRoute() is a recurring source of confusion for developers (c.f. #1988 recently, as well as this archive of past issues in general).

workbox.routing.registerNavigationRoute() appears to be a general-purpose method of registering a route that matches navigation requests, when in fact, it's implemented as a way of bridging the gap between workbox-routing and workbox-precaching, making it easier to handle navigations using precached URLs.

Not everyone wants to handle navigations with precached URLs, though.

I think in v5, we might be able to get away with moving most of the logic in registerNavigationRoute() from workbox-routing to workbox-precaching, and have it just return logic that could be used as a handler callback. This handler callback could then be used when constructing a route.

If we moved this logic to workbox-precaching we could also avoid having to explicitly call workbox.precaching.getCacheKeyForURL(), which has been another source of confusion in Workbox v4.

// v4:
workbox.routing.registerNavigationRoute(
  workbox.precaching.getCacheKeyForURL('/app-shell.html')
);

// v5:
const handler = workbox.precaching.createHandlerForURL('/app-shell.html');
const navigationRoute = new workbox.routing.NavigationRoute(handler);
workbox.routing.regsiterRoute(navigationRoute);

CC: @philipwalton who was looking into the feasibility of using workbox-strategies under the hood for the handler logic in workbox-precaching, as this proposed change might impact that plan.

i use the v5 solution u write is not work, below this one is work fine not sure why

const handler = workbox.precaching.createHandlerBoundToURL("/index.html");
const navigationRoute = new workbox.routing.NavigationRoute(handler);

workbox.routing.registerRoute(navigationRoute);

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

Successfully merging a pull request may close this issue.

4 participants