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

[editorial] dfn worker's outside/inside port, and clarify event retargeting #10738

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

Conversation

nicolo-ribaudo
Copy link
Contributor

@nicolo-ribaudo nicolo-ribaudo commented Nov 3, 2024

This started as a PR just adding <dfn> to a Worker's outside port, so that the algorithms using it could link to the definition.

However, when reading the definition I then noticed this sentence:

All messages received by that port must immediately be retargeted at the Worker object.

It has two problems:

  • there is no definition of what "retargeting an event" means. Actually, DOM does define it, but it's used when it comes to events related to the shadow dom and it would be a no-op on Worker/MessagePort.
  • as far as I can tell, not all events from MessagePort are forwarded. Specifically, the close event (which is fired when terminating the worker) is not included in the Worker event handlers table (and there is also an open feature request about it: Event for worker termination #5869). This is correct, because there is no algorithm that can end up triggering a "close" event on that message port, but it could be more explicit. Figuring out that no algorithm can trigger it requires noticing that a "disentangle the ports" step is not linked the "steps to disentangle a port".

So now this PR also explicitly dispatches the message and messageerror events on Worker/DedicatedWorkerGlobalScope rather than "retargeting" them from the MessagePort.

Question about the wording I chose: calling "dispatch an event" is enough to change the event's .target from the MessagePort to the Worker, right?


/index.html ( diff )
/web-messaging.html ( diff )
/webappapis.html ( diff )
/workers.html ( diff )

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this and I agree it's needed, but I'm afraid it doesn't quite work. E.g., I strongly expect event's "dispatch flag" to still be set at this point. I think we probably have to recreate the event object instead.

<var>options</var>)</code> on the port, with the same arguments, and returned the same return
value.</p>
<var>options</var>)</code> on <span>this</span>'s <span>outside port</span>, with the same
arguments, and returned the same return value.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point this should really be defined to invoke some internal algorithm instead of the public method.

@annevk annevk added clarification Standard could be clearer topic: workers labels Nov 4, 2024
@annevk
Copy link
Member

annevk commented Nov 4, 2024

cc @wanderview @asutherland

@nicolo-ribaudo
Copy link
Contributor Author

E.g., I strongly expect event's "dispatch flag" to still be set at this point.

Yes, it is. However, does it matter? Dispatch an event sets the flag unconditionally as the first step anyway.

@annevk
Copy link
Member

annevk commented Nov 4, 2024

Doh, only dispatchEvent() is guarded. But stop propagation and such seems like it would matter? Or maybe even that does not matter as nothing else can get a hold of this event object as the object that does the forwarding is not reachable to script. Hmm.

@smaug---- thoughts?

@nicolo-ribaudo
Copy link
Contributor Author

I think it does not matter because there is only one listener on that internal object, and the event does not bubble up because we are not in the dom. Capturing/bubbling order is also fine: the internal event listener runs in the "bubbling" phase, but then when dispatching it to the worker it goes again through the capturing and bubbling phases.

@nicolo-ribaudo nicolo-ribaudo marked this pull request as draft November 5, 2024 00:59
@nicolo-ribaudo
Copy link
Contributor Author

I'll make this better tomorrow

@nicolo-ribaudo nicolo-ribaudo changed the title [editorial] dfn worker's outside port, and clarify event retargeting [editorial] dfn worker's outside/inside port, and clarify event retargeting Nov 5, 2024
@nicolo-ribaudo nicolo-ribaudo marked this pull request as ready for review November 5, 2024 11:18
@nicolo-ribaudo
Copy link
Contributor Author

@annevk I changed this to now store the EventTarget for the message/messageerror events on the MessageChannel itself, so that the MessageChannel can dispatch the events to the right target rather than having this "retargeting" indirection. Wdyt?

@smaug----
Copy link

MessageEventTarget concept seems reasonable.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great modulo wrapping concerns. I'm not entirely sure if we should preserve the IDs here. I'd also like at least one other person to review, which likely means we should wait for Domenic to get back next week. Hope that's okay.

This is a very nice incremental improvement to messages though, thanks for working on it!

source Show resolved Hide resolved
@nicolo-ribaudo
Copy link
Contributor Author

No rush, I don't have any concrete need for this patch and it was just a possible improvement I spotted while reading :)

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is amazing.

I do think we should preserve these IDs, as they are linked from MDN and probably elsewhere: https://developer.mozilla.org/en-US/docs/Web/API/MessagePort/message_event#:~:text=%23%20event%2Dmessage-,HTML%20Standard,%23%20handler%2Dmessageport%2Donmessage,-Browser%20compatibility

To do this, just add id="..." in appropriately-nearby places (inserting empty <span>s if necessary).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Standard could be clearer topic: workers
Development

Successfully merging this pull request may close these issues.

4 participants