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

registerRoutes don't accept subclass of Route #636

Closed
jmtt89 opened this issue Jun 16, 2017 · 4 comments · Fixed by #644
Closed

registerRoutes don't accept subclass of Route #636

jmtt89 opened this issue Jun 16, 2017 · 4 comments · Fixed by #644

Comments

@jmtt89
Copy link

jmtt89 commented Jun 16, 2017

Library Affected:
workbox-sw, workbox-routing

Browser & Platform:
all browsers.

Issue or Feature Request Description:
registerRoutes don't accept subclass of Route

import WorkboxSW from 'workbox-sw';
import {RegExpRoute, ExpressRoute} from 'workbox-routing';
import {QueuePlugin} from 'workbox-background-sync';

const workboxSW = new WorkboxSW();

workboxSW.precache([...]);

//backgroundSync
let bgQueue = new QueuePlugin();

// Network Stategies
const networkOnly = workboxSW.strategies.networkOnly({
  plugins: [bgQueue]
});
const cacheFirst = workboxSW.strategies.cacheFirst();

//RuntimeCache
workboxSW.router.registerRoutes({
  routes: [
    new RegExpRoute({
      regExp: new RegExp('^https://fonts.googleapis.com/*'),
      handler: cacheFirst
    }),
    new RegExpRoute({
      regExp: new RegExp('^https://code.jquery.com/*'),
      handler: cacheFirst
    }),
    //Device Endpoints
    new ExpressRoute({
      path: '/my/custom/route',
      handler: networkOnly
    })
  ]
});

When reporting bugs, please include relevant JavaScript Console logs and links to public URLs at which the issue could be reproduced.

** Console Output **

Uncaught assertion-failed: The 'routes' parameter should be an array containing one or more 'Route' instances.

** Function throw error **

function isArrayOfClass(a, b) {
  const c = Object.keys(a).pop(), d = `The '${c}' parameter should be an array containing one or more '${b.name}' instances.`;
  Array.isArray(a[c]) || throwError(d); // Here all Good
  for (let e of a[c])
    e instanceof b || throwError(d); //This throw error
}
e ->
        RegExpRoute$1 {handler: CacheFirst, method: "GET", match: function}
          handler: CacheFirst
          match:({url:e})=> {…}
          method: "GET"
          __proto__:Route$1
b
       class Route{constructor({match:a,handler:b,method:c}={})
       {this.handler=normalizeHandler(b),assert.isType({match:a},'function'),this.match=a,c?
       (assert.isOneOf({method:c},validMethods),this.method=c):this.met…
e instanceof b
        false
@jmtt89
Copy link
Author

jmtt89 commented Jun 16, 2017

Extra update:
if use registerRoute method (two params, path and handler), work's fine but i can't add a http method

   //Calls OK
    workboxSW.router.registerRoute(new RegExp('^https://fonts.googleapis.com/*'), cacheFirst);
    workboxSW.router.registerRoute('/my/custom/path/:id', networkFirst);

@jeffposnick
Copy link
Contributor

I'm pretty sure this is due to #385. We need to address that underlying issue.

@mikefowler
Copy link

@jeffposnick I've recently run into the same issue. Seems like #385 is pretty stale. Any avenue for me to help out here?

@jeffposnick
Copy link
Contributor

jeffposnick commented Jun 19, 2017

The tactical fix is to re-export/alias the vanilla workbox.routing.Route class definition as WorkboxSWInstance.router.Route, and then it could be constructed or subclassed directly.

@gauntface and I have had some discussions about more elaborate workarounds, but in the interest of unblocking folks, I'm inclined to go that route for now, and perhaps consider a more comprehensive change for the next major release.

I'll put together a PR for that and give folks a chance to raise any objections there.

After re-reading the initial request in this thread, and then realizing that WorkboxSWInstance.router is itself an instance of a class, I realize that what I just suggested isn't the right approach. I'll come up with something else in a straw-man PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants