webcomponents-cg / community-protocols

Cross-component coordination protocols
179 stars 12 forks source link

[context] are context objects needed? #19

Closed matthewp closed 1 year ago

matthewp commented 3 years ago

Reading the Context proposal one thing that sticks out to me is that it's a bit unclear what the purpose of the Context object is. It seems like it's used by a provider as a filter for contexts that it can supply. Assuming that's the case, wouldn't make the type of context being requested part of the event name instead?

ie, instead of "context-request" it becomes "context-request-theme" and a theme provider listens to that specific event rather than all context events.

benjamind commented 3 years ago

This was actually how the initial proposal was made. The issue with this is it's incredibly 'loose', and it puts the responsibility of checking types on the consumer. With a well known and published context object we have an actual dependency that can be documented and clearly typed and shared. This tightens up the protocol a little bit and I think is generally worth the extra overhead.

matthewp commented 3 years ago

Thanks for the response @benjamind. Can you explain more? I'm not sure I understand, as DOM events have types, for example a click is a MouseEvent. I'm no TypeScript expert but it seems possible to map an event name to the typed event class, such as is done here: https://github.com/microsoft/TypeScript/blob/main/lib/lib.dom.d.ts#L5562. I don't see why these event classes can't be shared the same way that a context object can.

Some downsides to the current context object approach is that it repeats some things that are built in to DOM events:

justinfagnani commented 3 years ago

The tradeoff here is that with separate event names you have to add event listeners for each context type, since you can't add wildcard event listeners. That in turn makes it hard to abstract over the context protocol. You can't make a React bridge as easily, or forward all context requests to a different subtree (as you might want to do with portal/projection patterns).

It also means that the context key is a string - the event name suffix, and this is what Ben was alluding to. With a string key I think we run the real risk of collisions. How many incompatible context-request-logger or context-request-theme events will there be? Having the context key be an object basically eliminates such collisions.

benjamind commented 3 years ago

Thanks @justinfagnani for clarifying those points.

@matthewp to address some of your other questions. Yes you could use an interface map to type a string key, but that then would require a dependency on a common package to ensure those types are properly followed (the map has to be defined somewhere) so this doesn't gain us any convenience over the object approach.

Your other concern regarding once is I think also misunderstanding the purpose of dispose and the application of the once modifier. While an event listener at the provider level could be applied with the once modifier, this puts control of the behavior on the provider not the consumer. In the context case we want a consumer to advertise if it is willing to accept a value once or multiple times. The dispose function is then a mechanism to allow the consumer to detach itself from a provider that is providing a value repeatedly.

The event apis don't really give us any mechanism to do this unless we modified the protocol to use events going in both directions (i.e. a consumer dispatches an event to request a context, a provider is returned in the events callback/payload, the consumer then attaches a listener to the provider, and the provider then emits events on change). This is indeed another approach the protocol could take, but I fear it produces an explosion in events, and more complexity in hookup for the basic case.

matthewp commented 3 years ago

@justinfagnani

The tradeoff here is that with separate event names you have to add event listeners for each context type, since you can't add wildcard event listeners. That in turn makes it hard to abstract over the context protocol. You can't make a React bridge as easily, or forward all context requests to a different subtree (as you might want to do with portal/projection patterns).

I'd love to see the proposal updated with specifics of these use-cases. The current approach adds a lot of boilerplate over event names and I think that complexity needs to be justified with explicit use-cases.

It also means that the context key is a string - the event name suffix, and this is what Ben was alluding to. With a string key I think we run the real risk of collisions. How many incompatible context-request-logger or context-request-theme events will there be? Having the context key be an object basically eliminates such collisions.

The current proposal has the same change of collisions. The context name, a string, is the only way to differentiate between context requests. There is no instanceof here. So in order to know if a request is one a provider can provide they need to change the name, so the collision has the same chance as putting it on the event name.

benjamind commented 3 years ago

With the current proposal there is no chance of collisions as Providers check the provided context object for reference equality. They do not introspect into the name property of the object, that is only provided as a debugging convenience.

matthewp commented 3 years ago

@benjamind

@matthewp to address some of your other questions. Yes you could use an interface map to type a string key, but that then would require a dependency on a common package to ensure those types are properly followed (the map has to be defined somewhere) so this doesn't gain us any convenience over the object approach.

Yeah I agree, they seem equally able to be typed, in which case I think the more simple and webby approach, events, is the way to go here.

The event apis don't really give us any mechanism to do this unless we modified the protocol to use events going in both directions (i.e. a consumer dispatches an event to request a context, a provider is returned in the events callback/payload, the consumer then attaches a listener to the provider, and the provider then emits events on change). This is indeed another approach the protocol could take, but I fear it produces an explosion in events, and more complexity in hookup for the basic case.

Yeah, I think this is perhaps better discussed in a separate issue. The callback approach makes it seem like the event is handled asynchronously. Another approach is for the provider to add a property (or properties) to the event. That could contain the context value and an eventtarget to listen to for changes to the value. I'll file another issue to discuss this idea.

matthewp commented 3 years ago

With the current proposal there is no chance of collisions as Providers check the provided context object for reference equality. They do not introspect into the name property of the object, that is only provided as a debugging convenience.

Ok, I got it, they need to have a reference to the same object in memory. This makes the API more complex in my mind. We have these things that seem to conflict in my mind, or be too close to serving the same purposes:

There are a lot of moving parts here. I think a much simpler version of this proposal can exist by removing the context object which is a source of most of the complexity. Just events and properties added to the event.

I don't think name collisions justifies the added complexity and duplication of logic that events already provide (once, removeEventListener, filtering). Events do not provide referential equality check but dealing with global naming schemes is already part of using web components, for example when registering an element name.

benjamind commented 3 years ago

I don't think name collisions justifies the added complexity and duplication of logic that events already provide (once, removeEventListener, filtering). Events do not provide referential equality check but dealing with global naming schemes is already part of using web components, for example when registering an element name.

The once behavior of events can only be taken advantage of by further complicating the protocol. To be clear what you are proposing is the following:

With this approach we have much more complicated logic for handling updating contexts, or contexts which are provided asynchronously, we essentially have two paths through which context is provided. We also have many more events being created and dispatched when contexts are updated frequently (Events are not free). Consumers also need to still manage a reference to the EventTarget so they can later unsubscribe, so this is equavalent to the dispose method in the current proposal.

This change does not solve the collision problem either, and since you still need to require a dependency to get strong typing on the string keys, it has not reduced dependencies or complexity in usage. While I agree we do already have to manage namespace collision issues in the Web Components world, I do not think we should add to this burden if it can be avoided!

I do not feel this proposal simplifies anything over what is currently in the proposal, and I think there's additional runtime overhead to the event subscription model being proposed that could likely become an issue in some high frequency update scenarios.

I agree that the initialValue property is perhaps confusing and am willing to remove this. The only thought I had when adding this was to allow a context to define a 'default' value that consumers could use if no context is provided by their environment. This to me helps us define a clearer contract to users of a specific context, "You can ask for this context, if nobody provides it to you, you should use this value directly". But agree this is more complication than is perhaps necessary.

justinfagnani commented 3 years ago

Ok, I got it, they need to have a reference to the same object in memory. This makes the API more complex in my mind.

Complexity is a problem if it's significant and doesn't pay for itself, but in this case I really do think it does. String-based name collisions aren't just possible, but likely. "logger", "theme", "user", etc., are just too logical and common of names to not be used incompatibly across multiple components.

The object-key API is not unique to this context protocol, and it's really not that much more "complicated". This is very similar to the React API (https://reactjs.org/docs/hooks-reference.html#usecontext), and complexity isn't a complaint I hear about that.

In React you create a context:

x-react-logger

export const loggerContext = React.createContext(defaultValue);

and can then retrieve a context value with it:

import {loggerContext} from 'x-react-logger';

const MyComponent = () => {
  const logger = useContext(loggerContext);
  // ...
};

That just doesn't seem complex to me, and you'll be able to do the same patterns with the context protocol:

x-wc-logger

export const loggerContext = Symbol('logger');
import {loggerContext} from 'x-wc-logger';

class MyElement extends LitElement {
  _logger = new ContextConsumer(this, loggerContext);
}
matthewp commented 3 years ago

Things appear less complex when you omit code required to implement them 😀. I assume the undefined ContextConsumer is abstracting this code:

this.dispatchEvent(
  new ContextEvent(coolThingContext, (coolThing, dispose) => {
    // if we were given a disposer, this provider is likely to send us updates
    if (dispose) {
      // so dispose immediately if we only want it once
      dispose();
    }
    // guard against multiple assignment in case of bad actor providers
    if (!this.myCoolThing) {
      this.myCoolThing = coolThing; // do something with value
    }
  })
);

That is very complex. Think of scenarios where you need the dispose but not inside of that callback. Now you need to store it somewhere (probably on your this, if you have one) and then later conditionally check if you have it to use it (in your disconnectedCallback for example).

My proposal would be:

let event = new ContextEvent('cool-thing');
this.dispatchEvent(event);

// The current value.
console.log(event.context.value);

// The context is an EventTarget
event.context.addEventListener('change', () => {
  // do something with the new value, if you want.
  console.log(event.context.value);
});

// Get the next value but only once. This is free because its an EventTarget, no special API required.
event.context.addEventListener('change', () => {
  // Get this new value.
}, { once: true })

Complexity is a problem if it's significant and doesn't pay for itself, but in this case I really do think it does. String-based name collisions aren't just possible, but likely. "logger", "theme", "user", etc., are just too logical and common of names to not be used incompatibly across multiple components.

I don't understand this argument in the context of HTML/DOM based APIs where string keys are used everywhere. Should custom elements not use events at all then? After all they could be named the same as other events. If I emit a load it very well might conflict with another element's load. I suppose we could use a generic "something-happened" event for everything our application does and then use referential equality to verify it's the right "something-happened" we care about but I haven't seen this proposed.

Instead we use the usual method of preventing conflicts when we fear them; namespacing. That applies here the same as anywhere else (other events, custom element names, etc). The event name context-request can conflict as well, of course.

matthewp commented 3 years ago

The proper solution for event name collisions is upstream changes to DOM to allow symbols as event name. There's an existing issue where a maintainer expressed interest: https://github.com/whatwg/dom/issues/331. You could do something like:

provider.js

export const eventName = Symbol('some-thing');

this.addEventListener(eventName, () => ...
import { eventName } from 'https://example.com/some/shared/place/events/some-thing.js';

this.dispatchEvent(new ContextEvent(eventName));
benjamind commented 3 years ago

We're getting a little off topic here (we're supposed to be discussing context objects as key vs strings, we're now on the topic of callback vs event driven approaches).

Having said that, lets try and get a little more specific about the proposed change...

Your proposal is using EventTarget provided by the Provider inside the event, with context value being set into the event:

import { coolThingContext } from 'cool-thing';

class CoolThingElement extends LitElement {
    constructor() {}

    connectedCallback() {
        super.connectedCallback();

        // we want a cool thing
        let event = new ContextEvent(coolThingContext);
        this.dispatchEvent(event);

        // we have to check the context was set, since it might not have been handled
        if (event.context) {
            // it was set on the event context property
            this.coolThing = event.context.value;

            // we need to store the context event target so we can unsubscribe
            this.coolThingContext = event.context;
            // and we need to store the function so we can correctly unsubscribe
            this.onCoolThingChanged = (value) => { this.coolThing = value; }
            this.coolThingContext.addEventListener('change', this.onCoolThingChanged)
        }
    }
    disconnectedCallback() {
        super.disconnectedCallback()
        if (this.coolThingContext) {
            this.coolThingContext.removeEventListener('change', this.onCoolThingChanged);
        }
    }
}

And our current proposal:

import { coolThingContext } from 'cool-thing';
class CoolThingElement extends LitElement {
    constructor() {}

    connectedCallback() {
        super.connectedCallback();

        // we want a cool thing
        let event = new ContextEvent(coolThingContext, (value, dispose) => {
            // we were given a value in the callback
            this.coolThing = value;
            // we have to store dispose so we can use it later
            this.coolThingDispose = dispose;
        });
        this.dispatchEvent(event);
    }
    disconnectedCallback() {
        super.disconnectedCallback()
        if (this.coolThingDispose) { this.coolThingDispose(); };
    }
}

Its incorrect to think that EventTarget results in less complexity at the point of use. You still need to handle the unsubscribe, and you need to hang onto the contexts provided EventTarget to do that as well as the subscribed function. While once does make the single 'wait for one more update' case easier, that is not a common use-case at all and multiple update is more likely with disconnect/connected callbacks driving the add/removal of the event. The dispose implementation is actually simpler here, resulting in one less thing to track.

It also has a significant performance advantage.

Here's a rudimentary benchmark comparing the use of EventTarget vs callbacks.. On my run of this callbacks come out around 30% faster than using events, and 15% faster than events which reuse the event object (a potentially confusing pattern?). But this doesn't tell the whole story, taking a look atthe profile below we can see that the event driven approaches result in spiking memory usage and garbage collection, while the callback approach has stable memory usage. We are already somewhat concerned with the usage of events to setup context in large hierarchies of context dependent components. Using events further to drive updates is only going to increase the problem.

image

Overall while I heartily agree that protocols should try and hew as close to familiar browser patterns as we can, in this case I feel the proposal as currently stated is the most simple and efficient we can currently get.

benjamind commented 3 years ago

If we want to optimize for the 'get a context once' case, then we might consider combining some of these ideas:

class CoolThingElement extends LitElement {
    constructor() {}

    connectedCallback() {
        super.connectedCallback();

        // we want a cool thing
        let event = new ContextEvent(coolThingContext);
        this.dispatchEvent(event);
        // if we only want the value once, then retrieve it from the event object
        this.coolThing = event.value;
        // but if we want updates, then we can pass the callback:
        this.dispatchEvent(new ContextEvent(coolThingContext, (value, dispose) => {
             this.coolThing = value;
             this.coolThingDispose = dispose;
         })
    }
    disconnectedCallback() {
        super.disconnectedCallback()
        if (this.coolThingDispose) { this.coolThingDispose(); };
    }
}

This complicates the Provider somewhat since it needs to do both checking for a given callback in the event, and setting the value into the event, but this does make the 'once' case simpler on the part of the Consumer, and we can then make dispose a required parameter of the callback function type. This does however mean that the Provider must satisfy this context request synchronously, it doesn't have the option of passing the value asynchronously unless the Consumer provided the callback function.

I personally think the main use for Context is a 'live binding' with multiple update, and suspect the 'once' case is actually rare. If we look at prior art on this such as React Context they only support the live binding from what I can tell, all Consumers are updated when the Providers value changes.

Thoughts on this @justinfagnani ?

justinfagnani commented 3 years ago

Things appear less complex when you omit code required to implement them 😀. I assume the undefined ContextConsumer is abstracting this code:

The goal of the protocols is to allow interoperable implementations. It is not necessarily to be used without any implementations at all. Just as React, Angular, etc., provide sugary APIs for interacting with their underlying Context/DI implementations, we presume that libraries - whether associated with a web component helper library like Lit, Stencil, etc., or not - will provide nicer APIs than raw events.

It's also a goal of mine, though lower priority, that at least that the raw protocol not be too difficult to use by hand, so striving for as much simplicity there as possible is good, but critical functionality and interop are higher priority than that.

Regarding string names vs object reference though, this is simply a matter of safety and correctness. Strings have value semantics and you can't be sure that two independent modules won't accidentally use the same string. References fix this with the only added complexity coming from needing to get a hold of the reference. This will usually be an import, which has the benefit of providing some kind of reference back to an object that can be documented as to what the context's type and contract are. I suspect with string keys we'd see that pattern often anyway, making the complexity complaint moot.

fgirardey commented 3 years ago

The problem I see with depending on object references is that we can't pass variables to it.

Let's say my LoggerService need to be instanciated that way: new LoggerService('AppRoot') so it can output [AppRoot] something in the console.

How can we tell to the provider that the requested LoggerService should be configured with a specific name depending where it is requested?

I first thought about augmenting context but if it becomes dynamic we will lost the object reference equality benefits.

May be we need context objects + payload objects sent in the ContextEvent? Or do you prefer ContextEvent subclassing?

benjamind commented 3 years ago

I feel like in this case you want to request via context a factory function, which can then take the arguments and return the appropriate logger. i.e. if you want an object to be returned parameterized, the provider is a provider of functionality which enables this, not a mechanism by which the parameters themselves are passed.

The object reference is only for identifying the provider and doesn't restrict what could be provided.

fgirardey commented 3 years ago

Oh damn that's great! I've not thought about it that way, a factory function is indeed the best way to do that. Thank you for the clarification 🙏

renoirb commented 3 years ago

(new here, experimented a for a while before joining here. Gotta read all of what's around on the topic)

I get we should use Event instead of CustomEvent, with a normalized string context-request event name. Otherwise we would end up having to create as many global HTMLElementEventMap and lots of other things that would make it hard to be re-usable.

See: Snippet of an experiment using CustomEvent and use them with this proposal.

export interface AnotherEventContext {
  // ...
}

declare global {
  interface HTMLElementEventMap {
    readonly 'context-request:another-event': CustomEvent<AnotherEventContext>
  }
}

Making use of a plain Event, only one context-request and attach a context to that event makes sense. Dispatching from the component itself at connectedCallback also makes sense. Somehow, the component is a "consumer" telling how to keep itself consumed. Toy, blueprint, and batteries "included".

But for the subscribtion. The providers, how is it planned we set them up?

Should we have the Provider catch the context, and from that side (e.g. in the project code, no longer from lit adapter, or etc.) and from there, add other project side related DOM events, and how to cleanup etc? Something like "respond for" and "keep track" and "unregister". Like a "Stateful Context Manager" (see Gist)

That's what's still vague to me. — Probably I should read up more here :)

justinfagnani commented 3 years ago

@renoirb the proposal already uses a subclass of Event: https://github.com/webcomponents/community-protocols/blob/main/proposals/context.md#the-context-request-event

This is a separate topic from the context object question though, so I'd suggest a new issue if that doesn't answer your question.

renoirb commented 3 years ago

@renoirb the proposal already uses a subclass of Event: https://github.com/webcomponents/community-protocols/blob/main/proposals/context.md#the-context-request-event

Yes, I understood that bit. It was my way of giving context (pun not intended) to my question regarding what we pass in that Event. We basically pass more than a plain-old-javascript-object.

And we use what we pass in so that provider has access to mutating that consumer's (component) internal state. Consumer components might expose private methods, etc.

This is a separate topic from the context object question though, so I'd suggest a new issue if that doesn't answer your question.

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.

(moved to #21 in https://github.com/webcomponents/community-protocols/issues/21#issuecomment-905147691)

I won't continue in this thread if you're still certain of what I'm saying doesn't make sense. Maybe that should be a different thread. If so, please guide me in creating a re-worded description of this, please :)

benjamind commented 3 years ago

I think you have the right idea mostly @renoirb, though I think this is off-topic to the issue here, so I've filed #21 to continue the discussion there.

manolakis commented 3 years ago

I think that using object references could be problematic in some scenarios.

In my case we are building a big app composed by a shell app and several verticals as microfrontends (using web components), having each vertical its own build. Ideally, verticals could have context consumers while the shell app could contain the context providers. With object references all verticals must be aligned in order to use the same context references. Otherwise, consumers are going to ask for a non existent provider because the context reference is different between the vertical and the shell app.

I know this could be a strange use case now butr, the use of tools like skypack could make this use case more common in a near future.

benjamind commented 3 years ago

That's a good callout. If the context object key was made using Symbol.for('my-context') would that solve this case? Since the Symbol returned would then indeed be unique and consistent across the page.

manolakis commented 3 years ago

Indeed, that would work 👌

benjamind commented 3 years ago

Great, I can definitely make that change to the proposal. Will try and file a PR to that effect soon.

manolakis commented 3 years ago

@benjamind did you have time to try this?

I've been playing around using Symbol.for(key) and I don't see the difference between using it or just strings. I guess the effect is the same, and we lost the advantages of using context references.

So I think we could use something external to the protocol like a global registry for those cases.

export const getMycontext = () => getGlobalRegistry().get('my-context');
benjamind commented 3 years ago

Right yes had some time for hunk on this some more and my initial suggestion for Symbol.for will cause conflicts to become an issue once again. In discussion with @justinfagnani we concluded that the context request protocol should probably be non specific in what a context key should be. Some use cases may prefer global uniqueness with the downsides of potential conflict, others may prefer value semantics with the need to resolve shared keys via common imports. It feels like there is not a one size fits all solution here?

manolakis commented 3 years ago

Makes sense. Thanks!

matthewp commented 3 years ago

Note that things are progressing smoothly on the symbol-as-event-name front: https://github.com/whatwg/dom/issues/331#issuecomment-966640378

EisenbergEffect commented 2 years ago

A bit late here but chiming in from FAST's perspective.

We're working on integrating Context with our Dependency Injection system. In order to make that work, we prefer a single event that we can register the container for, and a specific context object that we can use to look up in the container. With an unknown list of events, things would be near impossible to handle in a generalized way. We'd have to force a ton of setup on consumers and would likely defeat the point of some of this.

A bit more technical details on our implementation: the DI system uses a Key to resolve dependencies. Key is defined as follows today (may change slightly in the next major release):

export type Key = PropertyKey | object | InterfaceSymbol | Constructable | Resolver;

The InterfaceSymbol type implements the Context interface, so we can use them interchangeably for Context API requests as well as direct DI requests (there's a factory that creates these with the createContext signature). PropertyKey includes Symbol as well, so we can handle Symbol for Context and DI the same way. Really, the DI system doesn't care what the key is but it does need some sort of key and we do need a way to intercept any/all context requests for a given DOM tree.

justinfagnani commented 2 years ago

The title of this issue, though not the multiple event names discussion, highlights an important discrepancy between the protocol as checked in and the @lit-labs/context implementation landed by @benjamind

In @lit-labs/context, createContext() is an identity function and it only casts it's argument as a ContextKey where:

export type ContextKey<KeyType, ValueType> = KeyType & {__context__: ValueType};

This actually seems sufficient for al use cases I can think of (except that it needs a brand property typed as never). I don't yet see why the context should define an initial value and not the context provider(s).

Should we remove the Context object from the proposal and say a context can be any value? Or should we update lit-labs/context to match?

An important question for me is whether there are concrete and critical use-cases for initial value that can't be met by the provider.

cc @benjamind

benjamind commented 2 years ago

My feeling is that a default value as defined by a context key isn't really that useful overall.

Defaults I have found to be component-local for the most part (what the component would use if the context was never provided). This is especially true where contexts are treated more like interfaces to be implemented by the providers, in which case what would a default value really be as defined by the interface?

You can also easily define a default value by simply having a default context provider at the root of your tree that would be superceded by a later registered context provider.

So my vote is to update the protocol to this, but I have to admit my head has not been in this space for a long time now.

gund commented 1 year ago

Should we remove the Context object from the proposal and say a context can be any value? Or should we update lit-labs/context to match?

From my perspective a context key should be treated as an opaque token/value by the spec and probably context API implementations as well. It's up to the context users to decide what a key should be in their use-cases.

justinfagnani commented 1 year ago

This is already the case in the Lit implementation, and I have a PR up to codify that change: https://github.com/webcomponents-cg/community-protocols/pull/36

renoirb commented 1 year ago

I think that using object references could be problematic in some scenarios.

In my case we are building a big app composed by a shell app and several verticals as microfrontends (using web components), having each vertical its own build. Ideally, verticals could have context consumers while the shell app could contain the context providers. With object references all verticals must be aligned in order to use the same context references.

Oh, @manolakis, if only I could have said exactly those words back when I came here. This was exactly what I was trying to explain and was working with (I no longer work there anymore though). How verticals can declare their shapes, and the shell to fulfill them.