-
Notifications
You must be signed in to change notification settings - Fork 190
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
Pause prefetches when non-prefetch request is sent to origin #45
Conversation
Updated per prior comments, and tests added. |
b641e99
to
2f2c0de
Compare
2f2c0de
to
8d73312
Compare
service-worker/bootstrap.js
Outdated
// until the request finishes, then resumes prefetching | ||
abortPrefetches() | ||
event.respondWith( | ||
fetch(event.request).then(resp => { |
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 think you need to also resume in a cache block here, so that prefetches can be resumed if the last fetch fails.
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.
Added a catch, and a test for that case.
service-worker/bootstrap.js
Outdated
resumePrefetches() | ||
} | ||
return resp | ||
}) | ||
.catch(e => { | ||
if (toResume.size) { | ||
resumePrefetches() |
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 guess resumePrefetches can be moved to finally, then there would be no code duplication and no need for catch clause, maybe I am wrong
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.
In order to do that, we would need to rewrite this using async/await instead of promises, which we should, as I believe those are natively supported in the service worker.
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.
# Conflicts: # package-lock.json # package.json
Fixed the merge conflicts, this should be good to go now. |
No description provided.