webcomponents-cg / community-protocols

Cross-component coordination protocols
179 stars 12 forks source link

[context] do providers retain references to consuming components? #21

Closed benjamind closed 1 year ago

benjamind commented 3 years ago

Moving this conversation here.

My question is still related about the Provider. Its lifecycle, how we should work when we're implementing one. I see @benjamind comment made on July 7th makes sense.

What feels unclear is if we have to have the Provider to hang on to that context-request Event reference, and make it so that it uses it when it detects it's time to disconnect

benjamind commented 3 years ago

I think you have the right idea.

It may be helpful to check out the lit-context implementation I've started here. Especially this ContextProvider implementation.

Assuming you wish to create a Context Provider which can deliver the context more than once, i.e. when its context value changes, it will have to hold onto the callback function which is provided in the ContextEvent for any context it is interested in satisfying, which also has the multiple property in the event set to true (indicating its willing to receive the value more than once).

It shouldn't retain the Event object, only the callback from within it. Whenever the context value in the provider changes (not properties on the context value, the value reference itself), the provider should call the callbacks it has collected again.

One thing that I think is wrong in your implementation here is that your provider is driving the requestUpdate call on the consuming component. This I think is violating an encapsulation rule. The context provider should know nothing about the rendering lifecycle of a context consuming component, it should merely provide the value in the callback, and leave updating to the consumer.

Additionally, if a ContextProvider intends to satisfy context requests with new values like this, it should pass a dispose method as a second argument to the callback, which the consuming component will invoke when it no longer wishes to receive values. The provider should then remove the callback from its collection.

https://github.com/webcomponents/community-protocols/blob/main/proposals/context.md#usage

renoirb commented 3 years ago

Moving here what was in https://github.com/webcomponents/community-protocols/issues/19#issuecomment-905135476 for webcomponents/community-protocols#19

renoirb: What feels unclear is if we have to have the Provider to hang on to that context-request Event reference, and make it so that it uses it when it detects it's time to disconnect.

(...)

Like a "Stateful Context Manager"; knowing which ContextAPI components are currently live, are accepting updates, etc.

/**
 * In experiment above, this was called UpdatableHonk — Naming things are hard.
 * Pretty much an event with a target property.
 */
export interface IContextEventWithTarget<T extends UnknownContext> extends IContextEvent<T> {
  readonly target: EventTarget
}

export class StatefulContextManager {
  contexts = new Map<string, Set<IContextEventWithTarget<Context<unknown>>>>()

  // Partially inspired by Microsoft Fast Foundation listenerMap
  // packages/web-components/fast-foundation/src/utilities/match-media-stylesheet-behavior.ts
  private listenerMap = new WeakMap<EventTarget, ContextCallback<unknown>>()

  respondFor(name: string, data: unknown) {
    const contexts = this.contexts
    if (contexts) {
      if (contexts.has(name) === false) {
        throw new Error(`StatefulContextManager respondFor: There is no context on the name ${name}`)
      }
      const entries = contexts.get(name)
      for (const { context, target, ...rest } of entries) {
        const callback = this.listenerMap.has(target) ? this.listenerMap.get(target) : void 0
        const payload = data ?? context.initialValue
        callback(payload)
      }
    }
  }

  protected keepTrackContextRequest<T extends UnknownContext>(event: ContextEvent<T>) {
    const { context } = event
    const { name } = context
    const contexts = this.contexts
    const callback: ContextCallback<ContextType<T>> = (value, dispose) => {
      event.callback(value, dispose)
      // communicate somehow that we've been into a change callback. Maybe?
    }
    // ... some way of calling the call back and updating
    const honk: IContextEventWithTarget<T> = {
      multiple: false,
      ...event,
      callback,
      context,
      target: event.target,
    }
    this.listenerMap.set(event.target, callback)
    if (contexts) {
      if (contexts.has(name)) {
        contexts.get(name).add(honk)
      } else {
        contexts.set(name, new Set([honk]))
      }
    }
    event.stopPropagation()
  }

  /**
   * So we can make the Provider to add the handler (remove below)
   * Making this Stateful Manager separate from the providers themselves.
   */
  addEventListenerTo(host: HTMLElement) {
    host.addEventListener('context-request', this.keepTrackContextRequest.bind(this))
  }
  removeEventListenerTo(host: HTMLElement) {
    host.removeEventListener('context-request', this.keepTrackContextRequest.bind(this))
  }
}

(Added while making this comment on this thread)

Not necessarily to do updates, like we see below with requestUpdate (yes it is violation — That is copy-pasta from experiments from a month ago).

benjamind: It shouldn't retain the Event object, only the callback from within it (...)

Right.

benjamind (...) One thing that I think is wrong in your implementation here is that your provider is driving the requestUpdate call on the consuming component. This I think is violating an encapsulation rule. The context provider should know nothing about the rendering lifecycle of a context consuming component, (...)

Right. And that callback may have access to internal state.

benjamind (...) it should merely provide the value in the callback, and leave updating to the consumer.

Exactly, we might still need some orchestration between consumer/provier, for making sure we properly removeEventListener, and try to get that "value in the callback"

Because, in the end, a provider would still have to know which components are "context-request" friendly, and may want to ask some registry that something needs to be changed.

How would that be done?

benjamind commented 3 years ago

No a provider doesn't need to lookup which components are context-request friendly. This is actually why moving to the context object as a key for the context event request is valuable.

When a component needs a context value, it will fire the ContextEvent, in that event payload is an object that is the shared agreed upon identifier for context values of the type the requesting component wants. Therefore if a provider which can satisfy that request intercepts the ContextEvent, it can be reasonably confident that the component that issued the event is playing by the Context API rules and will do the right thing with the value provided to the callback function that was attached to the event.

So there's no need for a registry, but there is need for a shared agreed upon object that is used to key the context-request event.

renoirb commented 3 years ago

(After having scavenged in notes)

What I've been talking here feels similar to what lit/lit#1955 is doing. You have an abstract "Provider" and "Consumer" (which also cleans after itself). Also a "ContextContainer" to handle telling that things has to be updated. — Which is close to what I've called here StatefulContextManager, hehehe, like benjamind/lit-context 🖖🏾 or DeadWisdom’s Gist, there's also @kcmr ’s kcmr/context-provider that looks similar too.

I hope my messy comments makes sense. In the links here, we might want an initial implementation that can be used as a starting point.

renoirb commented 3 years ago

No a provider doesn't need to lookup which components are context-request friendly. This is actually why moving to the context object as a key for the context event request is valuable.

OK! Makes sense.

That's why you have in benjamind/lit-context provide and the other to couple them. Nothing in the middle. No registry of sorts.

So there's no need for a registry, but there is need for a shared agreed upon object that is used to key the context-request event.

Right, we're doing the binding ourselves in both places instead of something doing it.

renoirb commented 3 years ago

If we consider making a dependency less, not bound to a framework package that provides Context API, we might want to provide a ContextProvider or ContextContainer. At least the boilerplate of it.

When making a package, we would ask package users to use the baked container,provider,consume in a way that would need to be extendable.

We could either provide that boilerplate as a class mixin or as "state manager" the component would instantiate at connectedCallback, or constructor time. Probably we could do the same for context-provider or context-container. So we have two bits who knows how to tie themselves together.

That side might be as useful as a "shared agreed upon object that is used to key the context-request event" because it would be the perfect place to enforce the contract.

What would that package do for this purpose?

PS: I tried to look for ContextAPI and web component communication protocol in W3C WICG discussion forum discourse.wicg.io , and haven't found anything and could be a good place to have a conversation. So that issue tracker conversation would be specification proposal draft process not a free-form conversation like I've been doing.

renoirb commented 3 years ago

Shoot, I missed today's meeting. I'll make sure it's in my calendar so I don't miss BREAKOUT: Community Protocols next time

renoirb commented 3 years ago

After more experimentation.

Keeping the callback reference for reuse makes sense.

What can we do if we have a good amount of Context callback in memory for the same Context. But each owner of that context reference might have a discriminant of some sort.

Say it's a property with an ID. The ContextConsumer is instantiated at constructor, we can't get that ID from the context. Or should we?

Ideas?


Adding a concrete example I wished I've brought up earlier, I've added it as comment in lit-labs context PR

What if we want many <simple-context-consumer></simple-context-consumer> with some way of discriminating each of them?

// Some list of IDs we already have
const ids = ['foo', 'bar', 'bazz']
// Iterating
const template = html`<simple-context-provider>
  ${repeat(
    ids,
    (id) =>
      html`<simple-context-consumer
        data-discriminant-id="${id}"
      ></simple-context-consumer>`,
  )}
</simple-context-provider>`

That pattern would be useful in a micro-front-end app that knows a list of ids, but can't know data to fill each <simple-context-consumer />.

justinfagnani commented 1 year ago

I feel like this is answered in the description of Context Providers here: https://github.com/webcomponents-cg/community-protocols/blob/main/proposals/context.md#context-providers

To quote an answer I gave in #39 :

providers do need to retain references to consumers if 1) the provider can update its 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.

justinfagnani commented 1 year ago

In https://github.com/webcomponents-cg/community-protocols/pull/40 I clarify the language to say that a provider must not retain a reference to the callback if subscribe (the new name) is not true.

Westbrook commented 1 year ago

Looks like we've resolved this with #40. Feel free to open a new issue if there's more focused discussion that remains.