Change add/remove event listener behavior for service workers #653

jungkees · 2018-06-06T03:07:47Z

This changes addEventListener() and removeEventListener() to not throw even
after the very first evalution of the service worker script. Instead, this
specifies user agents have to show a console warning that the asynchronously
added listener's event type will not affect service worker's behavior with the
functional event stored during the first evaluation.

This change referneces the funcional event concept defined in Service Workers
spec, and confines the EventTarge object of the specified behavior to
ServiceWorkerGlobalScope object instead of all the platform object in the global
object.

Related SW issue: w3c/ServiceWorker#1004.
Related SW PR: w3c/ServiceWorker#1322.
Fixes #371.

Preview | Diff

annevk · 2018-06-06T07:13:44Z

@domfarolino what is your expected timeline for fixing whatwg/console#57?

domfarolino · 2018-06-06T07:16:31Z

Under two weeks to be safe. I can prioritize that, thanks for the ping.

jungkees · 2019-07-17T22:27:31Z

@annevk, please take a look. I pushed a commit that uses the Console standard.

jakearchibald · 2019-07-19T10:33:06Z

dom.bs

-  <p class="note no-backref">To optimize storing the event types allowed for the service worker and
-  to avoid non-deterministic changes to the event listeners, invocation of the method is allowed
-  only during the very first evaluation of the service worker script.
+  <p>If <var>listener</var> observes a <a>functional event</a>, <var>eventTarget</var> is a


Feels clearer to say "If listener's type is…" then link to the list https://w3c.github.io/ServiceWorker/#execution-context-events.

Right now "observes" is a little vague, and "functional event" doesn't cover the lifecycle events.

DOM uses "observe" as in:

An event listener can be used to observe a specific event

If we use listener's type, we might need to compare it with the event's type attribute value, which makes it something like:

If listener's type is the type attribute value of one of the events in this list, ...

I'm fine with either, but also think that we might be able to drop this first condition entirely and just start with:

If eventTarget is a ServiceWorkerGlobalScope object, ...

This condition basically covers the listener's type is one in the list. WDYT?

and "functional event" doesn't cover the lifecycle events.

Good point!

jakearchibald · 2019-07-19T10:45:52Z

dom.bs

+  worker">script resource</a>'s <a for="script resource">has ever been evaluated flag</a> is set,
+  then the user agent must <a>report a warning to the console</a> with a description that explains
+  that the user agent stores the event <a for="event listener">type</a> of the <a>functional
+  event</a> for the <a>service worker</a> only during the very first evalution of the <a>service


Should the explanation be developer-focused here?

If so:

…the user agent must report a warning to the console explaining that an event with a type of listener's type should be added synchronously, otherwise, then the service worker restarts, events of this type will be dispatched before the listener is added.

I updated it. Just kept the wording,"with a description", to denote an argument and tried to explain the service worker will not start with the asynchronously added event.

jakearchibald · 2019-07-19T10:47:14Z

dom.bs

+  <p class="note no-backref">To optimize storing the event types allowed for the <a>service
+  worker</a>, Service Workers specification does not store the asynchronously added event <a
+  for="event listener">type</a>. The <a>event listener</a> will still be added under this condition,
+  but the implmentations have to inform developers about this behavior.


We might not need this note if the developer-facing explanation is clear enough.

I guess this note would help implementers understand why this behavior was added (while the warning description says what to display to the console as you pointed). I tried to simplify the words.

jakearchibald · 2019-07-19T10:54:48Z

dom.bs

+ resource">has ever been evaluated flag</a> is set, then the user agent must <a>report a warning to
+ the console</a> with a description that explains that the user agent will still start the
+ <a>service worker</a> for the <a>functional event</a> that has been stored during the very first
+ evalution of the <a>service worker</a> script and is asynchronously removed. [[!SERVICE-WORKERS]]


(any changes to addEventListener should also be made here)

I updated this accordingly too.

This introduces the concept, service worker events, to refer to the events that are one of lifecycle events, functional events, and the legacy message events dispatched to ServiceWorkerGlobalScope. Related PR: whatwg/dom#653.

jungkees · 2019-07-20T00:44:12Z

@jakearchibald, I uploaded a new snapshot. Please take another pass. This change references a change that I made in w3c/ServiceWorker#1452.

This introduces the concept, service worker events, to refer to the events that are one of lifecycle events, functional events, and the legacy message events dispatched to ServiceWorkerGlobalScope. Related PR: whatwg/dom#653.

jakearchibald

LGTM

annevk

Thanks, mostly nits from me.

annevk · 2019-08-13T08:56:07Z

dom.bs

-  only during the very first evaluation of the service worker script.
+  <p>If <var>eventTarget</var> is a {{ServiceWorkerGlobalScope}} object and its associated
+  <a>service worker</a>'s <a for="service worker">script resource</a>'s <a for="script resource">has
+  ever been evaluated flag</a> is set, then the user agent must <a>report a warning to the


then <a>report ... No need to say must here. (Also applies below.)

Done.

Out of my own curiosity, what'd be the actual meaning of it in terms of the conformance requirement after this change?

annevk · 2019-08-13T08:56:40Z

dom.bs

+  <a>service worker</a>'s <a for="service worker">script resource</a>'s <a for="script resource">has
+  ever been evaluated flag</a> is set, then the user agent must <a>report a warning to the
+  console</a> with a description explaining that the <a>service worker event</a> should be added
+  synchronously, otherwise the user agent will not start</a> the <a>service worker</a> for the


This looks like a stray </a>. I'm surprised that builds.

annevk · 2019-08-13T08:57:35Z

dom.bs

+  <p>If <var>eventTarget</var> is a {{ServiceWorkerGlobalScope}} object and its associated
+  <a>service worker</a>'s <a for="service worker">script resource</a>'s <a for="script resource">has
+  ever been evaluated flag</a> is set, then the user agent must <a>report a warning to the
+  console</a> with a description explaining that the <a>service worker event</a> should be added


"is to be added" (let's not use "should" as it's meant to indicate requirements to the implementer within this document).

annevk · 2019-08-13T08:58:24Z

dom.bs

+  <a>event</a>. [[!SERVICE-WORKERS]]
+
+  <p class="note no-backref">To optimize with the set of <a>service worker events</a> to handle, the
+  Service Workers specification does not store the asynchronously added event. The <a>event


It might be better to say what object does not store the event.

annevk · 2019-08-13T08:58:35Z

dom.bs

+  <a>event</a>. [[!SERVICE-WORKERS]]
+
+  <p class="note no-backref">To optimize with the set of <a>service worker events</a> to handle, the
+  Service Workers specification does not store the asynchronously added event. The <a>event


No newlines inside <a>.

jungkees · 2019-08-14T17:01:39Z

Thanks for reviewing, @annevk. I addressed your comments and uploaded a new snapshot. Please take a look.

annevk

Via some indirection user agents must implement the algorithm as written. Any must statement in the algorithm is redundant with that. A "should" statement or some such would work however, although that's not applicable here.

annevk · 2019-08-15T13:47:07Z

dom.bs

+ <a>service worker</a>'s <a for="service worker">script resource</a>'s
+ <a for="script resource">has ever been evaluated flag</a> is set, then
+ <a>report a warning to the console</a> with a description explaining that the user agent will still
+ start the <a>service worker</a> for the <a>service worker event</a> that was synchronously added.


We don't really know whether it was added at the correct time, right?

We are able to know whether the event has been added to the service worker's set of event types to handle. But I thought that we'd want to show the warning message when the function is called outside of the first evaluation regardless of whether the event has been added synchronously.

The event yes, but not this specific listener.

Thinking over it again, I think what we want to achieve for removeEventListener() is to warn, regardless whether it's called sync or async, removing the listener won't effect the service worker start-up if the service worker event was added in the script evaluation. Update the text in the follow-up commit.

annevk · 2019-08-15T13:51:52Z

dom.bs

+  <a>service worker event</a> is to be added synchronously, otherwise the user agent will not start
+  the <a>service worker</a> for the <a>event</a>. [[!SERVICE-WORKERS]]
+
+  <p class="note no-backref">To optimize with the set of <a>service worker events</a> to handle,


This doesn't read well. I also don't think we should use "synchronous" and "asynchronous" as it's not really clear relative to what they're being used. I'm not really sure what to suggest that doesn't get very wordy though.

I tried to rephrase the note by using "deterministically added .. during the very first script evaluation".

dom.bs

Description: Based on the spec (whatwg/dom#653), addEventListener after the first evaluation of the Service worker script should not throw. Change-Id: I46af276a67e021cf277a98195bf5c04583ad0b0a

erik-anderson · 2019-10-03T22:51:23Z

dom.bs

+  <p class="note no-backref">To optimize with the set of <a>service worker events</a> to handle,
+  asynchronously added <a>service worker events</a> are not added to the <a>service worker</a>'s
+  <a for="service worker">set of event types to handle</a>. The <a>event listener</a> will still be
+  added under this condition, but the implmentations have to inform developers about this behavior.


"implementations" is misspelled.

Fixed. Thanks.

Based on the spec (whatwg/dom#653), addEventListener after the first evaluation of the Service worker script should not throw. Change-Id: I46af276a67e021cf277a98195bf5c04583ad0b0a

Based on the spec (whatwg/dom#653), addEventListener after the first evaluation of the Service worker script should not throw. Change-Id: I46af276a67e021cf277a98195bf5c04583ad0b0a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1838376 Reviewed-by: Matt Falkenhagen <[email protected]> Commit-Queue: Daniel Soromou <[email protected]> Cr-Commit-Position: refs/heads/master@{#702753}

jungkees · 2019-10-07T18:28:58Z

I uploaded a new snapshot. Please take a look.

Also note that @grandfather1 added a test for this: web-platform-tests/wpt@bc28ef5. Thanks!

annevk

I wonder if we can say something simpler here. Something along the lines of adding/removing listeners after script evaluation might not give the expected result.

dom.bs

annevk · 2019-10-08T11:27:35Z

dom.bs

+ <a>service worker</a>'s <a for="service worker">script resource</a>'s
+ <a for="script resource">has ever been evaluated flag</a> is set, then
+ <a>report a warning to the console</a> with a description explaining that the user agent will still
+ start the <a>service worker</a> for the <a>service worker event</a> that was synchronously added.


The event yes, but not this specific listener.

annevk · 2019-10-08T11:29:40Z

dom.bs

+  <a>service worker</a>'s <a for="service worker">script resource</a>'s
+  <a for="script resource">has ever been evaluated flag</a> is set, then
+  <a>report a warning to the console</a> with a description explaining that the
+  <a>service worker event</a> is to be added synchronously, otherwise the user agent may not start


We don't know that it's a service worker event.

Right. I added a condition to do it only for service worker events in the follow-up commit.

I think keeping it vaguer as I suggested might be nicer, since if you know what you're doing, adding event listeners late (for your own dispatch logic) is okay. Seems rather odd to do something like that, but I also don't really see reason to depend on so many details of service worker logic here for a warning.

jungkees · 2019-10-08T19:23:07Z

I gave it another try. Please take a look. (I'd like to come up with simpler explanation but needs clarity about the behavior. Open to better language.)

The build is failing due to the reference to ServiceWorkerGlobalScope's service worker that I introduced. I made a PR to Service Workers to resolve it: w3c/ServiceWorker#1476.

…1476) whatwg/dom#653 needs a reference to this dfn.

…ronously doesn't throw., a=testonly Automatic update from web-platform-tests Service Worker: Fetch event added asynchronously doesn't throw. Based on the spec (whatwg/dom#653), addEventListener after the first evaluation of the Service worker script should not throw. Change-Id: I46af276a67e021cf277a98195bf5c04583ad0b0a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1838376 Reviewed-by: Matt Falkenhagen <[email protected]> Commit-Queue: Daniel Soromou <[email protected]> Cr-Commit-Position: refs/heads/master@{#702753} -- wpt-commits: 42efecfdb246ea0f22b5a82f5c13659894de3d72 wpt-pr: 19496

…ronously doesn't throw., a=testonly Automatic update from web-platform-tests Service Worker: Fetch event added asynchronously doesn't throw. Based on the spec (whatwg/dom#653), addEventListener after the first evaluation of the Service worker script should not throw. Change-Id: I46af276a67e021cf277a98195bf5c04583ad0b0a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1838376 Reviewed-by: Matt Falkenhagen <falkenchromium.org> Commit-Queue: Daniel Soromou <fosoromomicrosoft.com> Cr-Commit-Position: refs/heads/master{#702753} -- wpt-commits: 42efecfdb246ea0f22b5a82f5c13659894de3d72 wpt-pr: 19496 UltraBlame original commit: 85284299266e6cd046f12ed3a588f973b6013590

This changes addEventListener() and removeEventListener() to not throw even after the very first evalution of the service worker script. Instead, this specifies user agents have to show a console warning that the asynchronously added listener's event type will not affect service worker's behavior with the functional event stored during the first evaluation. This change referneces the funcional event concept defined in Service Workers spec, and confines the EventTarge object of the specified behavior to ServiceWorkerGlobalScope object instead of all the platform object in the global object. Related SW issue: w3c/ServiceWorker#1004. Related SW PR: w3c/ServiceWorker#1322. Fixes whatwg#371.

- Update the explanation to be more developer-focused - Use the newly defined service worker events concept

- Adjust the conformance requirement expression - Clarify the struct that stores events - Fix markup and alignment

- Reflect skipping event dispatch in Service Workers is optional - Rephrase notes - Fix a typo

annevk · 2019-11-01T14:12:16Z

@jungkees I've made the changes I suggested above and I also moved the removal warning to "remove an event listener" as it should probably apply to event handlers equally. Please let me know what you think. Ideally we'd merge this early next week.

jungkees · 2019-11-01T17:59:42Z

dom.bs

+  the expected results. [[!SERVICE-WORKERS]]
+
+  <p class="note no-backref">Service workers are only started for event listeners added during the
+  first script evaluation.


We have an occasion that is not covered by this note. Service Workers changed the skipping behavior to optional: https://w3c.github.io/ServiceWorker/#should-skip-event-algorithm step 1 uses "may". So, the listeners added after the first script evaluation still can see the service worker starts. Should we say: "Service workers may only be started for event listeners added during the first script evaluation?"

Why was that changed?

I ended up removing the note as the situation is complex and explaining it in detail here feels rather distracting for these general purpose methods.

I think the change was made to match the implementation. @mattto, can you give us more context on why you used may in that change?

Yes, I think it was mostly to match the implementations. It was the consensus at the F2Fs according to w3c/ServiceWorker#1200 (comment) and w3c/ServiceWorker#1200 (comment).

Limiting it to fetch seems okay, but making it random effectively requires user agents to look at each other's code if sites take a dependency. That's not great.

I've reopened the issue to track this.

jungkees · 2019-11-01T18:28:54Z

dom.bs

+ <a>report a warning to the console</a> that this might not give the expected results.
+ [[!SERVICE-WORKERS]]


To give the context of what the expected results would be, does leaving a short note along the lines of "Service Workers will still be started after the removal." make sense?

jungkees · 2019-11-01T18:34:03Z

@annevk, thanks for helping on this. It reads a lot simpler, indeed. I left two comments. I'm happy to see it merged early next week.

annevk added the needs tests Moving the issue forward requires someone to write tests label Jun 6, 2018

jakearchibald reviewed Jul 19, 2019

View reviewed changes

jungkees mentioned this pull request Jul 20, 2019

Define service worker events w3c/ServiceWorker#1452

Merged

jakearchibald approved these changes Aug 9, 2019

View reviewed changes

annevk reviewed Aug 13, 2019

View reviewed changes

annevk reviewed Aug 15, 2019

View reviewed changes

mfalken reviewed Sep 17, 2019

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

chromium-wpt-export-bot mentioned this pull request Oct 3, 2019

Service Worker: Fetch event added asynchronously doesn't throw. web-platform-tests/wpt#19496

Merged

erik-anderson reviewed Oct 3, 2019

View reviewed changes

annevk reviewed Oct 8, 2019

View reviewed changes

jungkees mentioned this pull request Oct 8, 2019

Export ServiceWorkerGlobalScope's service worker for DOM Standards w3c/ServiceWorker#1476

Merged

mfalken pushed a commit to w3c/ServiceWorker that referenced this pull request Oct 9, 2019

Export ServiceWorkerGlobalScope's service worker for DOM Standards (#…

32d2b5a

…1476) whatwg/dom#653 needs a reference to this dfn.

jungkees added 7 commits November 1, 2019 14:57

Define console reporting in terms of Console

9f77cba

Address comments

59b0f4e

- Update the explanation to be more developer-focused - Use the newly defined service worker events concept

Address comments

994530a

- Adjust the conformance requirement expression - Clarify the struct that stores events - Fix markup and alignment

Address comments

f328cf2

- Reflect skipping event dispatch in Service Workers is optional - Rephrase notes - Fix a typo

Correct the conditions for add/remove listeners; fix a typo

6b977b5

Fix punctuation

351fd52

annevk force-pushed the jungkees/event-listener-behavior-on-sw branch from 343c5c1 to 351fd52 Compare November 1, 2019 13:58

simplify a bit and move things around

128d23f

annevk mentioned this pull request Nov 1, 2019

Editorial: add missing coma in attachShadow #681

Merged

annevk removed the needs tests Moving the issue forward requires someone to write tests label Nov 1, 2019

jungkees commented Nov 1, 2019

View reviewed changes

remove inaccurate note

8946b24

annevk merged commit 7a48d64 into whatwg:master Nov 4, 2019

jungkees mentioned this pull request Nov 4, 2019

Behavior of the onfetch setter regarding no-fetch service workers w3c/ServiceWorker#1004

Closed

mfalken mentioned this pull request Nov 5, 2019

is the browser required to start the ServiceWorker for postMessage() if there is no message event handler? w3c/ServiceWorker#1200

Open

		<a>report a warning to the console</a> that this might not give the expected results.
		[[!SERVICE-WORKERS]]

Change add/remove event listener behavior for service workers #653

Change add/remove event listener behavior for service workers #653

Conversation

jungkees commented Jun 6, 2018 • edited by pr-preview bot Loading

annevk commented Jun 6, 2018

domfarolino commented Jun 6, 2018

jungkees commented Jul 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jungkees commented Jul 20, 2019

jakearchibald left a comment

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jungkees commented Aug 14, 2019

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jungkees commented Oct 7, 2019

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jungkees commented Oct 8, 2019

annevk commented Nov 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk Nov 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jungkees commented Nov 1, 2019

jungkees commented Jun 6, 2018 •

edited by pr-preview bot

Loading

annevk Nov 5, 2019 •

edited

Loading