- 
                Notifications
    You must be signed in to change notification settings 
- Fork 859
feat(workbox-precaching) Support for concurrent prefetching #3205
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
base: v7
Are you sure you want to change the base?
Conversation
| Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. | 
| entries: Array<PrecacheEntry | string>, | ||
| options?: PrecacheRouteOptions, | ||
| routeOptions?: PrecacheRouteOptions, | ||
| controllerOptions?: PrecacheControllerOptions, | 
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.
Not a big fan of the two option arguments. Combining them into a single options argument in a non-breaking manner didn't sit well with me either.
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'm curious why the additional arguments couldn't be just added to PrecacheRouteOptions? Or rather, could just concurrentRequests be added to PrecacheRouteOptions? Then getOrCreatePrecacheController could be updated to just be passed the one object with concurrentRequests argument extracted from the options. This would avoid the need for a second options object and it wouldn't have any back-compat breakage as far as I can tell.
Fix lint Add tests for concurrent precaching Improve documentation
80eabca    to
    b9d275a      
    Compare
  
    …ture/concurrent-precache
| any chance that this open PR gets more attention? Its still an issue on larger SPAs where the reload takes minutes of time until reloading is possible. | 
| @westonruter I would love to hear if there is any chance to get this released otherwise because it is still a major issue for users waiting for the files being pre-cached | 
| @dohomi Sorry, so far I've been focused on getting outdated dependencies updated. I haven't had time to work on new features. | 
| No problem I understand you guys have other priorities. I migrated to https://serwist.pages.dev/ just in case other people also struggle with long delays of precaching. Thanks for the heads up | 
Fixes #2880
Added the ability to precache requests concurrently. It looks like this used to be a feature in #2528, but was removed because it had no limits to the number of concurrent connections, resulting in a lack of resources for large sites with hundreds of assets.
This feature is opt-in, and should be non-breaking. Though it looks like this ticket is part of the V7 milestone. It may be possible to just bump a minor version for this.
Let me know if there's anything I missed that you'd like to see. One thing that might be interesting in the future is a dynamic concurrent request feature, where the lack of HTTP/2+ support or browser resource errors results in the throttling of the batch sizes.