-
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
v3: workbox-range-requests module #1108
Conversation
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. Only nits on descriptions / JSDocs
import './_version.mjs'; | ||
|
||
/** | ||
* A class implementing the `cachedResponseWillBeUsed` lifecycle callback. |
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 description should cover the purpose of the class, not the implementation details.
The range request plugin makes it easy for a Request with a 'range' header to be forfilled by a cached response.
It does this by intercepting thecachedResponseWillBeUsed
plugin callback and returning the appropriate subset of the cached Response body.
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.
Done.
* @param {Request} options.request The original request, which may or may not | ||
* contain a Range: header. | ||
* @param {Response} options.cachedResponse The complete cached response. | ||
* @return {Promise<Response>} If request contains a Range: header, then |
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.
Nit, could you shorten this to: "If request contains a 'Range' header, then a new Response with status 206 whose body is a subset of cachedResponse
is returned, otherwise cachedResponse
is returned as-is."
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.
Done.
PR-Bot Size PluginChanged File Sizes
New Files
All File SizesView Table
Workbox Aggregate Size Plugin☠️ WARNING ☠️We are using 150% of our max size budget. Total Size: 22.03KB Gzipped: 8.77KB |
R: @addyosmani @gauntface
Fairly straightforward port of the
workbox-range-requests
code from v2, with some test changes to accommodate the mocked interfaces, and some additional error messages.I updated the
workbox-sw
mapping, but I might be missing some other "meta" things that need to be changed when we add a new library.