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

[context] Fully event driven context protocol #39

Open
gund opened this issue Feb 2, 2023 · 7 comments
Open

[context] Fully event driven context protocol #39

gund opened this issue Feb 2, 2023 · 7 comments

Comments

@gund
Copy link

gund commented Feb 2, 2023

Hi, I recently discovered this proposal and it already has quite a nice approach.

However I've found that a few things in current state could potentially be improved by going fully event driven.

Basically right now only consumers dispatch events to "request" context which bubbles up to providers and then providers have to use attached callbacks to communicate back with consumers.
This introduces a bunch of problems like providers must store consumer's callbacks at all times which will force consumers to not being garbage collected and thus needing to invent another API surface to dispose/disconnect consumer's callback from a provider (which also opens up a room for a badly implemented providers that introduce leaks).

Since "request" is already an event - why not making a "response" an event as well (probably "provide" is a better name for event)?

This would require providers to only store consumer "HTMLElement" references which, if held in WeakRefs, would automatically be garbage collected whenever they are removed from DOM and no more other references are held, which in turn would fully eliminate the need for a disposal API.
This would allow to resolve #21.

Also the API with "dispose" callback seems quite awkward to work with (second optional argument in a callback, hello nodejs? xD ).
If the "provide" would happen as an event - there is no need at all for any cleanup API as consumer can simply remove event listener and be done with it.
We could even go deeper and define an optional event (like "context-unsubscribe" or "context-remove") to communicate to a provider that consumer is not interested in particular context anymore so provider may optionally do extra cleanup/optimisation.
This would allow to resolve #24.

I've just tried to implement fully event based context in my project and it works exactly the same as with callbacks in terms of calls order and sync guarantees but it allowed me to reduce API surface to a bare minimum (just events for context requests and provides).
I also implemented a fancier cleanup with extra event dispatched when a consumer is not interested in a context to further optimise communication but it is totally optional and may not be part of this spec.

You may check out my implementation here.

What do you think about fully event based Context API?

@justinfagnani
Copy link
Member

Firing an event on a specific target element, for that element to listen to, is very much like a callback, just way more heavyweight. This is an area where performance is a concern, and we already have overhead from the context request events - I would be very wary of doubling (or more) the number of events required to boot a page that depended heavily on contexts.

Events are also somewhat awkward for responses, in my opinion. It breaks the more common pattern with events where event are fired from a target for other actors to listen to. In this proposed form the event is fired on a target from another actor for the target to listen to.

And what do you get for using events for the response? I don't think you can get rid of dispose, because WeakRefs don't have the right semantics. You have absolutely no timing guarantees with WeakRefs, and an element could reattach to the tree and end up with multiple providers trying to satisfy context requests, some of which aren't ancestors anymore. That breaks the tree-scoping property we want from context. Disposal should be synchronous to disconnection for this reason.

If you could use WeakRefs to allow HTMLElements to be collected, you could also use WeakRefs to hold the callbacks. But you'd have the exact same timing problems.

#21 is already solved by the protocol: providers do need to retain references to consumers if 1) the provider can update it's context value, and 2) the multiple property of the request event is true. If both of those aren't true, then there's no need to hold a reference. If both are true, the dispose callback allows for severing the reference.

#24 is just a naming question and doesn't need to be solved with a different architecture. In any case, a new event needs a new name, so it's not like alternative are free from naming concerns.

@gund
Copy link
Author

gund commented Feb 2, 2023

I'm not sure on the exact penalties you would get if you will switch to an extra event (we should probably measure if it's at all significant) but it's possible to minimize it by preventing bubbling so it's only ever going to reach a single callback thus reducing overhead to a minimum.

Regarding the benefits I think you missed the point of a WeakRef as it's use-case is perfect for such a usage - we capture a weak reference to a DOM element and it will live there until it becomes unreachable, which will guarantee that it's not going to be removed from a WeakRef even between DOM re-attachments because someone somewhere will still hold it's reference.
And eventually when the element will become unreachable it will be safely garbage collected and cleaned up from the WeakRef.

In my implementation I've successfully got rid from any dispose APIs just because I hold DOM elements in WeakRefs and I have no callbacks that will cause memory leaks, so I'm not sure your points on WeakRef are correct.

@justinfagnani
Copy link
Member

I understand that WeakRefs make it seem like you can get rid of the dispose API, but you can't rely on WeakRefs in that way.

The relatively simple scenario that would demonstrate this is:

<parent-provider>
  <child-consumer></child-consumer>
<parent-provider>
<script>
  const parent = document.querySelector('parent-provider');
  const child = document.querySelector('child-consumer');

  parent.providedString = 'foo';
  // The child is receiving this value from the parent
  assert(child.providedString === 'foo');
  
  child.remove();
  
  parent.providedString = 'bar';
  // This should not update since the child is no longer in the subtree of parent,
  // but it will if we use a WeakRef approach because child is still in memory. So
  // this assertion will *fail*:
  assert(child.providedString === 'foo');
</script>  

The dispose callback (which I really think should be called unsubscribe as proposed in #24 ) needs to be synchronous with tree modifications to ensure the subtree semantics we want. So it needs to be called from disconnectedCallback().

Also, remember there is no guarantee that WeakRefs will ever be released in the lifetime of a program.

From MDN:

Correct use of WeakRef takes careful thought, and it's best avoided if possible. It's also important to avoid relying on any specific behaviors not guaranteed by the specification. When, how, and whether garbage collection occurs is down to the implementation of any given JavaScript engine. Any behavior you observe in one engine may be different in another engine, in another version of the same engine, or even in a slightly different situation with the same version of the same engine. Garbage collection is a hard problem that JavaScript engine implementers are constantly refining and improving their solutions to.

@gund
Copy link
Author

gund commented Feb 2, 2023

True, if you want to immediately stop receiving the updates of a context you need to "unsubscribe" but the beauty of the fully event driven approach is that consumer can simply remove event listener for receiving the context on disconnectedCallback and it will never get any updates even if a provider will still keep sending the events to it. So we basically invert the control of "subscription" from the provider (via callbacks) back to the consumer (via event listeners), who indeed is and should be controlling it.

For sure we can optimise this further by sending an event when consumer disconnects but it's purely optional and does not affect functional part.

Also events are dispatched and handled synchronously so there is no change in the calls flow/order.

The part about WeakRef guarantees is not so relevant here as the implementation does not and should not care when/if the refs will be cleaned up - if that happens then the memory will be released and that is all that we are trying to achieve - no direct references to hold back stale memory, so imo there is no problem is using WeakRefs as they were created specifically for such use-cases.

EDIT: On the note of sub-tree correctness I can actually think of the use-case where this proposal may yield undefined results: for example if we place child element under one provider and then move it under another provider - in this case same child element would receive 2 context values: one from the new provider + one from the old provider if it updates the value.
And to solve this we can make the "unsubscribe" event required instead of optional so that producers would remove consumer's WeakRefs from their memory as soon as those are disconnected.
But even still this approach would yield a nicer semantics and APIs as there will be no need for callbacks and even more callbacks for "unsubscriptions" - simply 3 events: "context-request", "context-provide" and "context-remove" 😊.

@matthewp
Copy link
Contributor

matthewp commented Nov 9, 2023

@gund I love the idea of a fully event driven context protocol and pushed for such a thing in the current proposal. One thing I'm curious about in your idea is, why do you need a response event? Why can't you attach the response to the current event? The provider would do:

let event = new ContextEvent(eventName)
el.dispatchEvent(event);
let result = event.result; // The consumer attached this.

@gund
Copy link
Author

gund commented Nov 9, 2023

@matthewp for the initial "request" phase of the context you do not need the "provide" event indeed but when you want to receive updates of the context you do need "provide" events as we do not want to handle callbacks from requestees.
So for the sake of simplicity and having APIs coherent it's better to use "provide" event from the start.

@mattlucock
Copy link

I have described an alternate design for a more event-driven protocol in #49.

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

No branches or pull requests

4 participants