-
Notifications
You must be signed in to change notification settings - Fork 312
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
spec says update()
should resolve to a registration, but browsers do different things
#1304
Comments
update()
should resolve to a registration, but browsers resolve to undefinedupdate()
should resolve to a registration, but browsers do different things
I have a WPT test in P2 here: |
I decided to just align FF with the spec and edge/safari. Our fix should be in FF61. It ultimately landed in this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1456466 The resulting WPT sync PR is: I filed a chromium issue about this here: https://bugs.chromium.org/p/chromium/issues/detail?id=836217 Closing this issue since it seems the spec is ok. |
Thanks for filing the chromium issue and adding the WPT! That's weird though... wouldn't reg.update() always resolve to reg? |
Pretty much, yeah. It doesn't really cause any harm, though, and could make it a bit more convenient to use. If people want to go the other way and align on undefined, I'm happy to do that too. The WPT has not been upstreamed yet. I ran into an unrelated issue in gecko I need to fix. |
I don't feel strongly. At least it seems right to upstream the WPT to match the spec. |
FYI, the WPT test has now merged upstream. |
Do we need to update the spec?
|
@leonhsl Good catch, yes that should be updated. |
See the uses of "Resolve Job Promise" in update and install algorithms:
https://w3c.github.io/ServiceWorker/#ref-for-resolve-job-promise%E2%91%A0
https://w3c.github.io/ServiceWorker/#ref-for-resolve-job-promise%E2%91%A1
They pass a registration to "Resolve Job Promise". That algorithm then says to expose it to script for either register or update jobs:
https://w3c.github.io/ServiceWorker/#resolve-job-promise-algorithm
AFAICT we don't have any WPT tests that actually assert the resolved value of
update()
. As a result we have an incompatibility:I'm inclined to conform to the spec and edge/safari here. And I can write a WPT test.
@jakearchibald @jungkees @mattto What do you think?
The text was updated successfully, but these errors were encountered: