fix(service-worker): allow checking for updates when constantly polling the server #40234
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gkalpak
added
type: bug/fix
action: merge
|
You can preview 6812895 at https://pr40234-6812895.ngbuilds.io/. |
gkalpak
force-pushed
the
fix-sw-idle-delay
branch
from
6812895
to
5954d49
Compare
|
You can preview 5954d49 at https://pr40234-5954d49.ngbuilds.io/. |
IgorMinar
approved these changes
Jan 5, 2021
| cacheNames | ||
| // The Chrome debugger is not able to render the syntax properly when the | ||
| // code contains backticks. This is a known issue in Chrome and they have an | ||
| // open [issue](https://bugs.chromium.org/p/chromium/issues/detail?id=659515) for that. |
There was a problem hiding this comment.
nit: this issue is now marked as fixed
There was a problem hiding this comment.
Nice! I removed the work-around 🎉
…out unrecoverable state Previously, the `Driver#notifyClientsAboutUnrecoverableState()` method would not wait for the completion of the promises created to notify the clients. Theoretically, this could result in the SW instance's getting destroyed by the browser before all clients have been notified. This is extremely unlikely to happen in practice, since the async operations are very quick, but it _is_ theoretically possible. This commit ensures that the SW instance will remain alive while notifying the clients by making `notifyClientsAboutUnrecoverableState()` await the notification promises.
Previously, clients were notified about updates sequentially. This wasn't necessary. This commit changes the `Driver#notifyClientsAboutUpdate()` method to notify the clients in parallel (by switching from `Array#reduce()` to `Array#map()` and `Promise.all()`). This also aligns the `notifyClientsAboutUpdate()` method with the `notifyClientsAboutUnrecoverableState()` method.
…river#deleteAllCaches()` This commit refactors `Driver#deleteAllCaches()` to use `Array#map()` instead of `Array#reduce()` for running async operations in parallel. This allows avoiding having to recursively wrap Promises with `Promise.all()`.
This commit refactors the `Driver#handleFetch()` method to not have to call `event.waitUntil(this.idle.trigger())` in multiple places.
…ng the server Previously, the SW would wait to become idle before executing scheduled tasks (including checks for newer app versions). It was considered idle when it hadn't received any request for at least 5 seconds. As a result, if the app performed polling (i.e. sent requests to the server) in a shorter than 5 seconds interval, the SW would never detect and update to a newer app version. Related issue: angular#40207 This commit fixes this by adding a max delay to `IdleScheduler` to ensure that no scheduled task will remain pending for longer than the specified max delay.
…ting bug Chrome debugger code highlighting bug [659515][1] has been fixed, so we can remove the work-around. (See angular#38332 for more details on the work-around.) [1]: https://bugs.chromium.org/p/chromium/issues/detail?id=659515
gkalpak
force-pushed
the
fix-sw-idle-delay
branch
from
5954d49
to
eaa7cf2
Compare
|
You can preview eaa7cf2 at https://pr40234-eaa7cf2.ngbuilds.io/. |
atscott
pushed a commit
that referenced
this pull request
Jan 11, 2021
…40234) Previously, clients were notified about updates sequentially. This wasn't necessary. This commit changes the `Driver#notifyClientsAboutUpdate()` method to notify the clients in parallel (by switching from `Array#reduce()` to `Array#map()` and `Promise.all()`). This also aligns the `notifyClientsAboutUpdate()` method with the `notifyClientsAboutUnrecoverableState()` method. PR Close #40234
atscott
pushed a commit
that referenced
this pull request
Jan 11, 2021
) This commit refactors the `Driver#handleFetch()` method to not have to call `event.waitUntil(this.idle.trigger())` in multiple places. PR Close #40234
atscott
pushed a commit
that referenced
this pull request
Jan 11, 2021
…ng the server (#40234) Previously, the SW would wait to become idle before executing scheduled tasks (including checks for newer app versions). It was considered idle when it hadn't received any request for at least 5 seconds. As a result, if the app performed polling (i.e. sent requests to the server) in a shorter than 5 seconds interval, the SW would never detect and update to a newer app version. Related issue: #40207 This commit fixes this by adding a max delay to `IdleScheduler` to ensure that no scheduled task will remain pending for longer than the specified max delay. PR Close #40234
atscott
pushed a commit
that referenced
this pull request
Jan 11, 2021
…ting bug (#40234) Chrome debugger code highlighting bug [659515][1] has been fixed, so we can remove the work-around. (See #38332 for more details on the work-around.) [1]: https://bugs.chromium.org/p/chromium/issues/detail?id=659515 PR Close #40234
atscott
pushed a commit
that referenced
this pull request
Jan 11, 2021
…out unrecoverable state (#40234) Previously, the `Driver#notifyClientsAboutUnrecoverableState()` method would not wait for the completion of the promises created to notify the clients. Theoretically, this could result in the SW instance's getting destroyed by the browser before all clients have been notified. This is extremely unlikely to happen in practice, since the async operations are very quick, but it _is_ theoretically possible. This commit ensures that the SW instance will remain alive while notifying the clients by making `notifyClientsAboutUnrecoverableState()` await the notification promises. PR Close #40234
atscott
pushed a commit
that referenced
this pull request
Jan 11, 2021
…40234) Previously, clients were notified about updates sequentially. This wasn't necessary. This commit changes the `Driver#notifyClientsAboutUpdate()` method to notify the clients in parallel (by switching from `Array#reduce()` to `Array#map()` and `Promise.all()`). This also aligns the `notifyClientsAboutUpdate()` method with the `notifyClientsAboutUnrecoverableState()` method. PR Close #40234
atscott
pushed a commit
that referenced
this pull request
Jan 11, 2021
) This commit refactors the `Driver#handleFetch()` method to not have to call `event.waitUntil(this.idle.trigger())` in multiple places. PR Close #40234
atscott
pushed a commit
that referenced
this pull request
Jan 11, 2021
…ng the server (#40234) Previously, the SW would wait to become idle before executing scheduled tasks (including checks for newer app versions). It was considered idle when it hadn't received any request for at least 5 seconds. As a result, if the app performed polling (i.e. sent requests to the server) in a shorter than 5 seconds interval, the SW would never detect and update to a newer app version. Related issue: #40207 This commit fixes this by adding a max delay to `IdleScheduler` to ensure that no scheduled task will remain pending for longer than the specified max delay. PR Close #40234
atscott
pushed a commit
that referenced
this pull request
Jan 11, 2021
…ting bug (#40234) Chrome debugger code highlighting bug [659515][1] has been fixed, so we can remove the work-around. (See #38332 for more details on the work-around.) [1]: https://bugs.chromium.org/p/chromium/issues/detail?id=659515 PR Close #40234
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, the SW would wait to become idle before executing scheduled tasks (including checks for newer app versions). It was considered idle when it hadn't received any request for at least 5 seconds. As a result, if the app performed polling (i.e. sent requests to the server) in a shorter than 5 seconds interval, the SW would never detect and update to a newer app version.
Related issue: #40207
This PR fixes this by adding a max delay to
IdleSchedulerto ensure that no scheduled task will remain pending for longer than the specified max delay.This PR also includes some minor refactorings/improvements. See the individual commits for more details.