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

Prohibit ServiceWorker event handler update after the initialization. #1161

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

yoshisatoyanagisawa
Copy link

@yoshisatoyanagisawa yoshisatoyanagisawa commented Feb 15, 2023

This is a very early stage proposal on the specification change. I hope to listen to what you think of the change.
I will update the following checklist afterwords.

  • At least two implementers are interested (and none opposed):
  • Tests are written and can be reviewed and commented upon at:
  • Implementation bugs are filed:
    • Chromium: …
    • Gecko: …
    • WebKit: …
    • Deno (only for aborting and events): …
    • Node.js (only for aborting and events): …
  • MDN issue is filed: …

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

@yoshisatoyanagisawa
Copy link
Author

@jakearchibald @LeszekSwirski
I have drafted the specification update upon the discussion. Please let me know your thoughts.
Thank you.

aarongable pushed a commit to chromium/chromium that referenced this pull request Feb 16, 2023
To understand the effect of whatwg/dom#1161,
let me add use counters to count updates of handlers prohibited by this
proposal.

Bug: 1347319
Change-Id: Ic6a19a512222dabca8e52416fdab9629ce9a70ad
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4259225
Commit-Queue: Yoshisato Yanagisawa <[email protected]>
Reviewed-by: Kouhei Ueno <[email protected]>
Reviewed-by: Minoru Chikamune <[email protected]>
Reviewed-by: Shunya Shishido <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1106099}
chirayudesai pushed a commit to chirayudesai/chromium that referenced this pull request Mar 7, 2023
To understand the effect of whatwg/dom#1161,
let me add use counters to count updates of handlers prohibited by this
proposal.

(cherry picked from commit c49519f)

Bug: 1347319
Change-Id: Ic6a19a512222dabca8e52416fdab9629ce9a70ad
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4259225
Commit-Queue: Yoshisato Yanagisawa <[email protected]>
Reviewed-by: Kouhei Ueno <[email protected]>
Reviewed-by: Minoru Chikamune <[email protected]>
Reviewed-by: Shunya Shishido <[email protected]>
Cr-Original-Commit-Position: refs/heads/main@{#1106099}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4273346
Auto-Submit: Yoshisato Yanagisawa <[email protected]>
Cr-Commit-Position: refs/branch-heads/5563@{#663}
Cr-Branched-From: 3ac59a6-refs/heads/main@{#1097615}
@annevk
Copy link
Member

annevk commented Mar 14, 2023

Did you look into the history as to why this was not throwing? It's not clear to me why we are revisiting that.

@yoshisatoyanagisawa
Copy link
Author

Maybe #371 ?

As far as I checked the history, this used to throw but changed to just warn.
7a48d64

@LeszekSwirski
Copy link

We're currently looking into this because of investigations into skipping the SW startup for trivial/simple handlers (in addition to the existing behaviour of skipping it for no handler). The thinking is that the possibility of dynamic changes to the set of listeners of an event will make any static analysis of the definition of "trivial" near impossible.

We weren't aware of the previous discussion, sorry about that and thanks for bringing it up. We'll catch up on the history there and will see if we have any new thoughts, arguments or evidence.

@jakearchibald
Copy link
Collaborator

Past discussion is here #371 (comment)

If the developer adds a service worker global event for "install", "activate", "fetch", "push" after the initial execution of the service worker, they're going to get behavior they don't expect. Worse, it might work as they expect in testing, but sometimes go wrong for real users.

The goal here is to avoid this case. Right now we show a console warning, which is ok, but could it be worth tightening the rules here so issues are found in development?

Option 1: Throw

I'd forgotten that we'd discussed that previously. We could limit the throwing to a subset of event types, but it seems that folks we still be against that.

Option 2: Ignore

Instead of throwing, the call could be ignored. This would cause it to fail in dev, and (hopefully) cause the developer to see the console warning, and amend their code. Again, this could be limited to particular event types.

In both cases, the behaviour should be applied to both event listeners and event handlers.

@annevk
Copy link
Member

annevk commented Mar 16, 2023

I prefer not changing this. If we got to do this again I'd probably push harder for service workers not using events, but now that they do use events they should stick to the event model as close as possible. Having certain known "types" be ignored or throw exceptions very much goes against that.

Contrived, I know, but you might very well have a custom push event you dispatch on the global or any other kind of known "type" we might add to the platform.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants