Conversation
…e allowing any operations on it.
🦋 Changeset detectedLatest commit: 2a81f96 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Collaborator
Size Report 1Affected Products
Test Logs |
dlarocque
reviewed
Dec 3, 2024
Comment on lines
+62
to
+76
| return new Promise<void>((resolve, reject) => { | ||
| if (registration.active) { | ||
| resolve(); | ||
| } | ||
| const incomingSw = registration.installing || registration.waiting; | ||
| if (incomingSw) { | ||
| incomingSw.onstatechange = ev => { | ||
| if ((ev.target as ServiceWorker)?.state === 'activated') { | ||
| incomingSw.onstatechange = null; | ||
| resolve(); | ||
| } | ||
| }; | ||
| } else { | ||
| reject(new Error('No incoming service worker found.')); | ||
| } |
Contributor
There was a problem hiding this comment.
I think it would be extremely unlikely, but if the registration transitions to the active state after if(registration.active) and before const incomingSw = registration.installing || registration.waiting, I think we would reject and the registration would fail.
Contributor
Author
There was a problem hiding this comment.
I changed it to an if/else but not sure if this affects the timing at all.
Contributor
There was a problem hiding this comment.
Yeah this is a tricky timing issue. I think now the SW could become active before we add the event listener, causing us to miss the activated event. For example:
- registration is waiting
- registration is not yet active, so we don't resolve immediately on line 79.
- registration becomes active, and the
'activated'event fires - attach registration state event listener
- event listener never fires because we missed the
'activated'event and we reject the promise after 10s
This is just me speculating, I'm not sure if this order of events is possible.
Contributor
Changeset File Check ✅
|
Collaborator
Size Analysis Report 1Affected Products
Test Logs |
dlarocque
approved these changes
Dec 4, 2024
Merged
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
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.
Examples on MDN seem to claim that
serviceWorkerRegistration.pushManager.subscribe()can be called right afterawaitingnavigator.serviceWorker.register, but that doesn't actually seem to be the case in practice. See issues #7784 and #7693.When the user calls
getToken()(and is using the default service worker), we awaitregister()inregisterDefaultSw()and we go ahead and callpushManager.subscribe()soon after. Unfortunately it's often not actually ready. Not sure if this is a bug - spec seems to indicate it should be ready?One option is to await
navigator.serviceWorker.readybut this can be tricky depending on the scope of the service worker, where it may not consider your service worker the page's default service worker. It seems to hang in my test apps. It also may get mixed up with other service workers from other apps on the same page.Instead, we can listen for events on the specific service worker we just registered and check when the
statebecomes "active". Borrowed some working code for this from various contributors in #7693 (comment) (Thank you!)Fixes #7784
Fixes #7693