control non amp pages via amp-sw #28
Conversation
| super(config); | ||
| private allowedPages_: Array<RegExp>; | ||
|
|
||
| constructor(config: { |
There was a problem hiding this comment.
Let's make the config an interface defined before the default class.
Also if it's by default required your can force all fields to be optional via TS syntax. Partial<Config>. (https://github.com/Microsoft/TypeScript/blob/ee25cdecbca49b2b5a290ecd65224f425b1d6a9c/lib/lib.es5.d.ts#L1354)
There was a problem hiding this comment.
Also, since this extends an interface from Workbox, let's make sure we're extending the type provided by Workbox.
If Workbox isn't providing the interface type, let's fix their definitions.
There was a problem hiding this comment.
Re Comment 2: The next version of Workbox is being written in TS, so that should solve the problem
| }) { | ||
| const { allowedNonAMPPages, ...pluginConfig } = config; | ||
| super(pluginConfig); | ||
| if (allowedNonAMPPages && Array.isArray(allowedNonAMPPages)) { |
There was a problem hiding this comment.
Possible to make this early exit pattern instead of checking existance in successive condition checks?
if (allowedNonAMPPages) {
if (!Array.isArray(allowedNonAMPPages)) {
throw new TypeError('allowedNonAMPPages should be an array of RegExp');
}
this.allowedPages_ = allowedNonAMPPages;
}| if (currentTestUrl.test(clonedResponse.url)) { | ||
| return response; | ||
| } | ||
| } |
There was a problem hiding this comment.
Might also consider an Array.find.
There was a problem hiding this comment.
fixed with Array.some as we were not using the returned object
| @@ -30,6 +30,7 @@ export type DocumentCachingOptions = { | |||
| timeoutSeconds?: Number; | |||
| maxDocumentsInCache?: Number; | |||
| maxAgeSecondsforDocumentsInCache?: Number; | |||
| allowedNonAMPPages?: Array<RegExp>; | |||
There was a problem hiding this comment.
Investigate extending the Plugin interfaces to create the DocumentCachingOptions interface.
There was a problem hiding this comment.
The reason why we cannot do this is because our keys do not match with the Plugin's config keys.
e.g. MaxExtries makes sense for workbox but for us the same key is known as MaxDocumentsInCache so the extends wouldn't work here
There was a problem hiding this comment.
Makes sense, thanks for investigating. Might recommend a comment or similar.
| super(pluginConfig); | ||
| if (allowedNonAMPPages) { | ||
| if (!Array.isArray(allowedNonAMPPages)) { | ||
| throw new TypeError('allowedNonAMPPages should be an array of RegExp'); |
There was a problem hiding this comment.
If the value is a single RegExp should this handle it?
There was a problem hiding this comment.
I guess just enforcing this to always be an array should make this intuitive enough for everyone, plus lesser switch cases around typeof would keep the code straight forward
| @@ -30,6 +30,7 @@ export type DocumentCachingOptions = { | |||
| timeoutSeconds?: Number; | |||
| maxDocumentsInCache?: Number; | |||
| maxAgeSecondsforDocumentsInCache?: Number; | |||
| allowedNonAMPPages?: Array<RegExp>; | |||
There was a problem hiding this comment.
Makes sense, thanks for investigating. Might recommend a comment or similar.
fixes #17
allowedNonAMPPages:Array<RegExp>config todocumentCachingOptions.