Skip to content
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

Strategy unhandled rejection #3320

Merged
merged 5 commits into from
Dec 11, 2024

Conversation

joshkel
Copy link
Contributor

@joshkel joshkel commented Apr 27, 2024

Fixes #3171

StrategyHandler.doneWaiting throws on the first rejected promise that it encounters. This means that subsequent rejected promises may result in unhandled rejection errors.

Promise.allSettled is a natural solution. It does not appear to be supported on all browsers that workbox uses. (However, I'm having trouble finding a clear statement of what browsers workbox supports, so I could be wrong.) I tried installing promise.allsettled from NPM but had trouble getting the default import from that module to work correctly with Rollup, and I noticed that the workbox service worker libraries tend to avoid external dependencies in general, so I created a local version instead. I put it under workbox-core's _private directory, following the example of other utility code such as Deferred. If I should handle this differently, please let me know.

This is a rebase of #3172 against the v7 branch.

@tomayac
Copy link
Member

tomayac commented Apr 30, 2024

Fully approve of the intent of the PR. However, I don't think Promise.allSettled() needs polyfilling looking at browser support. Looking at devices that didn't make the Safari 13 cut, this are iPhone 5S, iPhone 6 and 6 Plus, the original iPad Air, the iPad mini 2 and iPad mini 3, and the 6th-generation iPod Touch. For Chromebooks, we'd be locking out Chromebooks stuck on Chrome <76. @tropicadri, what do you think?

doneWaiting throws on the first rejected promise that it encounters.  This means that subsequent rejected promises may result in unhandled rejection errors.

Fixes #3171
@joshkel joshkel force-pushed the strategy-unhandled-rejection branch from 1d1c330 to f5c41d4 Compare November 19, 2024 19:54
@westonruter
Copy link
Collaborator

westonruter commented Nov 19, 2024

Fully approve of the intent of the PR. However, I don't think Promise.allSettled() needs polyfilling looking at browser support.

I'll note that Promise.allSettled() is now Baseline Widely available. So yeah, maybe no polyfill is needed?

@joshkel
Copy link
Contributor Author

joshkel commented Nov 19, 2024

Thanks for the feedback, @westonruter (and @tomayac). Since Promise.allSettled is widely available, it seems like no polyfill is needed. However, making TypeScript happy with Promise.allSettled will require changing tsconfig.json's lib from es2017 to es2020. Is it okay to do that as part of this PR?

@westonruter
Copy link
Collaborator

@joshkel Yes, that makes sense to me, since Promise.allSettled() was introduced in es2020.

As discussed in code review, this is widely available.  Update tsconfig.json to recognize it.
@joshkel
Copy link
Contributor Author

joshkel commented Nov 20, 2024

@westonruter I've switched to the native version. Thanks again.

let promise;
while ((promise = this._extendLifetimePromises.shift())) {
await promise;
while (this._extendLifetimePromises.length) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose the while loop needs to be retained in case additional promises are added to the array while awaiting the ones that were removed during the first iteration?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. (That was my rationale, at least.)

@westonruter
Copy link
Collaborator

On a related note, I just recalled that the Browser Support page indicates that only the current version minus 2 are to be supported. This is now clearly the case for Promise.allSettled()!

image

@westonruter westonruter merged commit 9c73193 into GoogleChrome:v7 Dec 11, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StrategyHandler.doneWaiting may result in unhandled promise rejections
3 participants