-
Notifications
You must be signed in to change notification settings - Fork 823
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
Support for registerRoute(<string>) #1028
Conversation
Changes Unknown when pulling f6b5037 on string-routes-in-v3 into ** on v3**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM, just some of the logic I think can be simplified and the Error thrown on express style stuff should be a warning message, not an error.
assert, | ||
cacheNames, | ||
logger, | ||
} from 'workbox-core/_private.mjs'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need to be changed to:
import {WorkboxError} from 'workbox-core/_private/WorkboxError.mjs';
import {assert} from 'workbox-core/_private/assert.mjs';
...
}); | ||
} | ||
|
||
// We want to only prohibit these characters in the pathname portion of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change this to "We want to check if express style characters are in the pathname only."
// See https://github.com/pillarjs/path-to-regexp#parameters | ||
const wildcards = '[*:?+]'; | ||
if (valueToCheck.match(new RegExp(`${wildcards}`))) { | ||
throw new WorkboxError('invalid-wildcards', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really think this should be a warning and not an error.
Also worth adding a TODO to remove this in v4
https://stackoverflow.com/questions/5649730/what-characters-can-one-use-in-a-url
|
||
const matchCallback = ({url}) => { | ||
// If we have a path-only capture pattern... | ||
if (capture.startsWith('/')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if statement feels error prone.
Wouldn't this have the same result:
// If capture is full URL, new URL will do nothing, if it's '/' it'll have same origin
return (url.href === new URL(capture, location).href);
You could nest a debug message to call out "the pathname matches but the origin is different so if you intended a match, please use the full URL".
# Conflicts: # packages/workbox-routing/_default.mjs # test/workbox-routing/node/test-default.mjs
PTAL. It should have pr-bot properly configured now as well. |
Changes Unknown when pulling b8bf6ba on string-routes-in-v3 into ** on v3**. |
Changes Unknown when pulling 99f6770 on string-routes-in-v3 into ** on v3**. |
PR-Bot Size PluginChanged File Sizes
New FilesNo new files have been added. All File SizesView Table
Workbox Aggregate Size Plugin☠️ WARNING ☠️We are using 126% of our max size budget. Total Size: 18.46KB Gzipped: 7.99KB |
Changes Unknown when pulling 99f6770 on string-routes-in-v3 into ** on v3**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Over to Matt for a confirm on review comments being addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - Thanks Jeff.
R: @addyosmani @gauntface
Fixes #1012