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] Some concerns, and an idea of how they could be addressed #49

Closed
mattlucock opened this issue Nov 16, 2023 · 16 comments
Closed

Comments

@mattlucock
Copy link

mattlucock commented Nov 16, 2023

This issue is fundamentally similar to #39 in that the author of that issue and I both agree that the protocol should be more event-driven, but we have very different ideas of how to go about it. In order to motivate my idea, I need to outline the problems I think it would solve. Most of this is me outlining the problems I think currently exist, and given there could exist alternative solutions to my concerns, I thought it apt to make a separate issue about the concerns and then also propose a solution.

Concerns about the current design

The callback doesn't make sense if the consumer isn't subscribing

If a consumer wants to synchronously request a context value one time, they have to do something like this:

getContextValue(context) {
  let value;

  this.dispatchEvent(
    new ContextEvent(
      context,
      (providedValue) => {
        value = providedValue;
      },
    ),
  );

  return value;
}

We know, with knowledge of the protocol, that this works, given that a provider will immediately call the callback, but it's actually not obvious from reading the code that this works; it's actually surprising that this works.

I also think it's just awkward. I suggest that the reason it's awkward is because a callback is simply an incorrect representation of the semantics. It's a callback that will be called, synchronously, only once; I don't think it makes sense.

The biggest problem with this, and possibly the biggest problem with the whole design, is that a provider can call a consumer's callback multiple times when the consumer only expects it to be called once. The proposal directly grapples with this, introducing the idea that providers are capable of being 'bad actors' and that consumers can be 'defensive'. I feel strongly that the fact this is even possible is not a necessary trade-off, but a flaw in the design; it doesn't make any sense that this is possible.

Another problem is that it's possible for the provider to cause a memory leak by retaining a consumer's callback when the consumer did not expect it to be retained. The proposal discusses this, of course, but I think it just shouldn't be possible.

The primary advantage I can see of using the callback for this in the current design is that it means the provider provides the value to the consumer in the same way regardless of whether the consumer is subscribing, but I think there's a better solution.

The current approach to unsubscribing is odd

In order to unsubscribe at a time of its choosing, the consumer has to fish the unsubscribe function out of the callback in the same way they have to fish the value out of the callback, which is awkward. I, again, think it's awkward because the consumer unsubscribing in this way is not a good representation of the semantics.

We want to return values to the consumer, but we also want to return an unsubscribe function to the consumer. The callback seems like a convenient way to do this, but in fact, it overloads the callback with responsibility; providing the value and providing the unsubscribe function are distinct responsibilities.

I think that consumers should unsubscribe using an AbortSignal. This would not only be a better representation of the semantics and a better separation of responsibility, but this approach is already in use with EventTarget and it would be good use of built-in APIs—which, after all, is what web components are about.

I note that it would be possible to allow this by having the consumer attach an AbortSignal to the context-request event within the constraints of the current design, but

  • Then there would be three properties that are all tied to subscriptions, the subscribe flag, the callback, and the signal.
  • The provider would be expected to test whether the signal is already aborted before they begin providing the context.

As an aside: does the provider pass the unsubscribe function every time they call the callback, or just the first time? The proposal doesn't actually say. If it has to be passed every time, I think that's undesirable for both the provider and the consumer.

How does a consumer know whether there exists a provider?

From what I can tell, if a consumer wants to know whether there exists a provider, the consumer needs to do something like this (which I note is basically the same as the code above):

doesProviderExist(context) {
  let providerExists = false;

  this.dispatchEvent(
    new ContextEvent(
      context,
      () => {
        providerExists = true;
      },
    ),
  );

  return providerExists;
}

This is awkward and confusing for the same reasons as above.

The proposal does not address how a consumer can detect whether a provider exists, which, to me, seems extremely important: a consumer requests data but has no idea if they'll ever receive it. Surely the consumer would want to handle the case where they won't.

My idea

I really like the idea of common, community context protocol, and I want this initiative to succeed, but I have serious reservations with it as it is currently specified. Here is my idea for an alternative design:

The provider implements a getContext(context) method that returns the value of the specified context in the provider, and also emits a context-change event (that doesn't bubble) whenever the value of one of the contexts it provides changes.

When a consumer requests a context, they emit a context-request event that bubbles up the DOM in order to 'discover' whether there exists a provider for the specified context. If a provider can provide the specified context, it stops the propagation of the event and sets itself as a property on the event (event.provider = this). This is similar in many ways to the existing context-request event described, but achieves something fundamentally different.

After the consumer emits the event, they can retrieve the provider from the event and handle the case where no provider exists. If the provider exists, they can call provider.getContext(context) to retrieve the value of the context. That's it. If the consumer wants to subscribe to the context, they can call getContext() to get the value of the context at the time of subscription, and then attach a context-change listener to the provider to become aware of any future values. My instinct is that event should only specify the context that changed, and not its new value, because this would give the consumer more than one correct way of retrieving the value (getting it from the event or getting it from the provider directly).

I think this is an overall simpler design that also addresses the flaws in the current design. The consumer easily detects whether a provider exists, and can retrieve a context once without any unnecessary complexity or pitfalls. If a consumer optionally wants to subscribe to a context, they can do so by attaching event listener, without concerns about how the provider will call the listener, whether the provider will retain it, how they will extract values from the callback, or how they will unsubscribe. I believe that this design would address the problems with the proposed solution in #39 while ultimately addressing the concerns originally raised in the issue. It would also comprehensively solve #21, which I feel still has not been genuinely solved.

Funnily enough, the proposal actually mentions a potential "alternate API" in which consumers emit an event to discover providers, but does not contemplate any reason why this might be advantageous other than type-safety concerns, which the proposal (in my opinion) rightly dismisses. From my perspective, this is a little frustrating.

Trade-offs

One trade-off with this design is that the consumer gains a reference to the provider, thereby gaining at least some knowledge of who the provider is. However, I think this design actually decreases coupling between the provider and consumers. In the current design, providers are forced to be aware of consumers and to deal with their callbacks. In my proposed design, providers have literally no knowledge of consumers. In TypeScript, the type of the provider would be represented in such a way that the consumer could not become coupled to the provider.

Another trade-off is that providers would need to specify in their context-change events what context changed, and consumers would need to inspect the event to verify that it's actually relevant to them. In the general case, just as most context-request events are not relevant to any given provider, most context-change events would not be relevant to any given consumer.

Example TypeScript

type ContextKey<Value> = (string | symbol | object) & { __context: Value };
type ContextValue<T extends ContextKey<unknown>> = T extends ContextKey<infer Value> ? Value : never;

type ContextProvider = {
    getContext<T extends ContextKey<unknown>>(context: T): ContextValue<T> | undefined;
    addEventListener(type: "context-change", listener: (this: ContextProvider, ev: ContextChangeEvent) => any, options?: boolean | AddEventListenerOptions): void;
    removeEventListener(type: "context-change", listener: (this: ContextProvider, ev: ContextChangeEvent) => any, options?: boolean | AddEventListenerOptions): void;
};

class ContextRequestEvent<T extends ContextKey<unknown>> extends Event {
    context: T
    provider?: ContextProvider

    constructor(context: T) {
        super("context-request", {
            bubbles: true,
            composed: true,
        });

        this.context = context;
    }
}

class ContextChangeEvent extends Event {
    context: ContextKey<unknown>

    constructor(context: ContextKey<unknown>) {
        super("context-change");
        this.context = context;
    }
}

interface HTMLElementEventMap {
    "context-request": ContextRequestEvent<ContextKey<unknown>>;
}

I've taken some liberties in how I've written this that aren't necessarily important. One issue I have with the code in the proposal is that createContext is a JavaScript function that exists only for TypeScript purposes; it returns a nice generic type, but it doesn't actually do anything. The Context type in the proposal requires us to specify the type of the key even though we don't actually care what its type is; we need to specify the type of the key just so that the key is assignable to the type of the context e.g. "my-context as Context<string, number>. If we were to restrict the type of keys ahead-of-time, even with a very loose restriction like string | symbol | object, specifying the type of the key becomes unnecessary e.g. "my-context" as ContextKey<number>.

Closing thoughts

I don't know whether any of this alternate design is actually of any use. Given that the proposal is now candidate status—as of, apparently, two weeks ago—it probably can't be redesigned in a meaningful way at this point, which is disappointing. I've only just discovered the WCCG and this proposal recently; if I had found this earlier, I would have participated earlier.

Regardless, I think the issues I've described are of real concern, and I would be keen to try and address these issues in any way that is feasible.

I'm very keen to hear any thoughts anyone has.

@benjamind
Copy link
Collaborator

I generally quite like this proposal, but one major concern that would need some prototyping and testing to evaluate is the cost of EventTarget based implementation of change events. The new Event construction is actually quite GC intensive, especially if a context value is under rapid change. I'm already a little concerned about performance of the current event protocol during initialization of large apps with deep trees and high use of context requests. Any event driven proposal I think needs to come with some performance testing at scale to show this isn't going to cause headaches for large apps.

@mattlucock
Copy link
Author

mattlucock commented Nov 16, 2023

@benjamind Only just now have I found #19 and seen you write, two years ago, about the cost of events. You say that such a proposal needs to come with performance testing, but it seems like you already have a good idea of what the results of that testing would be: events are more expensive. But I don't think that means we shouldn't use them.

As a compromise, providers could have a subscribeToContext method that takes an AbortSignal, which would theoretically provide many of the same benefits, and which I suppose would have the added benefit of consumers not being notified of contexts they don't care about. But it doesn't feel right to me.

Events are the API for communication in the DOM. Providers are effectively already EventTargets, except they're implemented manually in userland, and they're not as good. Contrary to what you wrote in that issue, I think not using events definitely adds complexity; providers have to manually store, call, and dispose of listeners. If the provider was an EventTarget, it wouldn't have to worry about any of that. Consumers could elegantly detach listeners using an AbortController, which does not require retaining a reference to the EventTarget. As far as rapid state changes are concerned: it's up to the provider to decide when to notify consumers of state changes. The provider could batch changes to state (which I expect would happen some of the time in practice anyway), or manually choose to defer notifying consumers until an appropriate time. This doesn't necessarily solve the problem, but I think it certainly would in at least some cases.

It seems strange that we would deliberately avoid using events and manually implement our own EventTarget-ish object instead; it doesn't feel like a good precedent to set. I think the real solution to this is not to propagate state changes using events at all and to instead return some kind of reactive state container like in #43. Until then, I think there's a good chance the benefits outweigh the trade-offs.

That said, the compromise approach I mentioned would still be a big improvement, I believe.

For what it's worth, I do believe that Events are great for user interactions with the DOM, and not so great for everything else. Earlier this year, I was doing some experimental work on event emitters, and what I wrote felt quite superior to EventTarget for actual software use cases . But EventTarget is the standard, built-in API that everyone has and everyone can agree on. It's certainly not perfect, but trying to implement a custom alternative to it didn't really feel like the right solution. Perhaps I'll reconsider.

@justinfagnani
Copy link
Member

I'd like to understand what the actual supposed problems are with the current protocol is and how the change is an improvement.

The callback doesn't make sense if the consumer isn't subscribing

Why not? This is a subjective claim that could use some more backing.

The callback is how the event listener is able to send information back to the event dispatcher. There are other ways to do this for sure, such as a simple property, but the callback is uniform across both the single request and subscription cases, which I think is a major benefit.

Even base events have similar ways of send data from listeners to dispatcher: preventDefault() called by a listener causes the defaultPrevented field to be set that the dispatcher can check.

The current approach to unsubscribing is odd

This is very subjective too. Odd to whom?

Again, the callback is a way for listener to send data to the dispatcher. One of the things that may need to be sent is the unsubscribe function. I don't find this odd.

How does a consumer know whether there exists a provider?

You showed exactly how.

a consumer requests data but has no idea if they'll ever receive it. Surely the consumer would want to handle the case where they won't.

Consumers can already do this if they don't receive a call to the callback, like you showed.

I would like to see claims of problems be more concrete. Like "can't do X", "performance is slow", "too much memory", etc. Right now it looks like your proposal is roughly similar in terms of capabilities, so I'd also like to see how it's better. I don't immediately see that it is.

I do actually see important ways in which it's limited in compared to the current protocol. Most critically it creates a tighter coupling between the consumer and provider objects, such that it would be very difficult (if not completely unsupported) to dynamically change the provider of subscriptions.

This is something that has come up in real-world usage of the protocol because of the dynamic nature of the DOM tree and custom element upgrades:

  • Consumers may dispatch the context-request event before the provider is upgraded
  • A new provider may upgrade lower in the tree than the previous provider
  • A tree change may cause the consumer to be a descendent of a different provider

The loose coupling between consumer and provider of the current protocol is a huge advantage here. The semantics of the subscribe: true events are "keep me updated with the current value of this context as it changes" regardless of whether the change is because the existing provider has a new value, or because the provider changed.

@mattlucock
Copy link
Author

mattlucock commented Nov 16, 2023

@justinfagnani

I'd like to understand what the actual supposed problems are with the current protocol is and how the change is an improvement.

Fortunately I spent several paragraphs trying to articulate exactly this. I must tell you with respect that it does not feel like you've actually cared to listen to most of the arguments I was trying to make, which is really very disheartening as an outsider interested in advancing web components. I'm very happy to have it explained to me why I'm wrong, but this just feels passive-aggressive.

I am not trying to argue that there is something the protocol should do which it currently doesn't; I'm trying to argue that, in my opinion, the design is confusing, is needlessly complex, has notable pitfalls, and poorly represents the semantics of what it's trying to do. If you're not interested in concerns of this nature, that's fine, but I'd prefer you to just say that.

The callback is how the event listener is able to send information back to the event dispatcher. There are other ways to do this for sure, such as a simple property, but the callback is uniform across both the single request and subscription cases, which I think is a major benefit.

I know. I realised this by myself. I explicitly acknowledged that this was the major advantage of the existing approach. It feels like you didn't even read this:

The primary advantage I can see of using the callback for this in the current design is that it means the provider provides the value to the consumer in the same way regardless of whether the consumer is subscribing...

In this section I tried to outline multiple issues with using the callback when not subscribing, including what I think is the biggest flaw with the whole protocol (as I said), which is that it's possible for 'bad actor' providers to exist. It's not apparent that you listened to anything I said here. I made no mention of a provider returning a context value to a consumer using a "simple property".

This is very subjective too. Odd to whom?

Again, the callback is a way for listener to send data to the dispatcher. One of the things that may need to be sent is the unsubscribe function. I don't find this odd.

You just haven't engaged with any of the arguments I tried to make here.

Edit: As with the above instance with the callback being used for both single requests and subscriptions, I also acknowledged what you said here in the original issue:

We want to return values to the consumer, but we also want to return an unsubscribe function to the consumer. The callback seems like a convenient way to do this...


Consumers can already do this if they don't receive a call to the callback, like you showed.

I tried to argue that this is confusing (the code does not seem like it should work, so it's surprising that it does), and is awkward and brittle to write. It's not at all obvious, using the protocol, that provider non-existence can be detected, and that this how you would do it. Provider non-existence is an important thing for a consumer to know, but the protocol fails to represent this to consumers; the protocol just expects consumers to work it out for themselves. As I said, the proposal doesn't even mention anything about this. Your indifference suggests this is by design; I think, in good faith, that that's a problem. (Edit: it's also the case that, because of what I believe are poor semantics, a consumer that subscribes and wants to check for provider existence is forced to confirm provider existence every time the value is provided, rather than just the first time).

As for the topic of consumers discovering providers and subscribing: it's especially discouraging that the one part of what I wrote that you've properly engaged with is the part you wanted to criticise in detail.

This is the part where I really might just have no idea what I'm talking about, and I'm happy to have it explained to me why I'm wrong, but I've read the proposal several times, and I can't see how any part of it describes how a provider of subscriptions can dynamically change. The context-request event is dispatched once, and it bubbles once; if no-one responds to it, no-one will ever respond. This was the whole idea behind the detecting provider non-existence code I showed, which you confirmed was correct. The proposal doesn't seem to describe any mechanism by which a provider can retrospectively listen for a request after the request was made—and indeed , the fact that the protocol doesn't provide a solution for "Consumers may dispatch the context-request event before the provider is upgraded" appears to be what #25 is all about. The proposal also doesn't seem to describe any mechanism by which a provider can become aware of a different provider and somehow 'transfer' existing subscriptions to that provider; you seem to be very strongly implying that it does.

The semantics of the subscribe: true events are "keep me updated with the current value of this context as it changes" regardless of whether the change is because the existing provider has a new value, or because the provider changed.

I have, again, read the proposal several times, and I am, again, happy to have it explained to me how I'm wrong, but it seems to me that is definitely not what the proposal says the semantics of subscribe: true are. The proposal says that subscribe: true means that the consumer wants the provider to call the callback again if the context value changes. It makes no mention of this flag allowing the provider to dynamically change.

* A boolean indicating if the context should be provided more than once.

If the event has a truthy subscribe property, then the provider can assume that the callback can be invoked multiple times, and may retain a reference to the callback to invoke as the data changes.

An element may also provide a subscribe boolean on the event detail to indicate that it is interested in receiving updates to the value.

Like you, I care about the web and want web components to succeed. I'm grateful for the work that has been done and is being done by the WCCG, including on this protocol.

@Westbrook
Copy link
Collaborator

@mattlucock thanks for sharing your thoughts! Lots of great ideas here that we look forward to getting to the bottom of.

Like @benjamind, I am interested in the nuances of this alternative approach but also have apprehension regarding the requirement of new Event() at scale. While I do anticipate the cost of new Event() to be higher, something I've proved in other contexts for myself, in all things there is a cost and it would be interesting to see what the delta would end up being between the current approach and yours. Luckily, this protocol has still just entered the "Candidate" status wherein:

Community members should use this opportunity to outline any final concerns or adjustments they'd like to see in the protocol before adoption.

This means you are just in time to join the convo! 😉 It is in use, in production, pretty widely (at least in projects I've related to), so it will be a pretty high bar to see changes, but, as you've noted, your reservations are "serious", so hopefully you'll be able to support the process of kicking the tires here, as it were.

I'm still processing all of the OP, but there are at least two main topics that I'd like to address: AbortSignal usage, and the alternate "subscription" model.

Re: AbortSignal

I like the idea of normalization with the platform in this protocol. Would you be amenable to a standalone issue/PR that outlined how this could/should be leveraged in the current protocol structure? It doesn't seem bound to your alternative API structure proposal, so I wouldn't want it to get lost if overall the community decided the current structure was the more appropriate approach. You mention some drawbacks there around the multiplicity of "subscribe" markers and "signal checking", but I think in isolation that we could easily ameliorate those issues and possibly have an even more clear API for the change.

Re: alternate "subscription" model

Some things I'd appreciate hearing more about her:

  • What are the practical costs of new Event() vs the current approach?
    I've had good success micro-benchmarking these sorts of differences with tachometer, @justinfagnani's team built this to specifically direct conversation with data, which seems more than appropriate in this case. You're probably right that many of us expect it to be slower, but I like being wrong for the betterment of a project, and a little slower for ease of use is different than a lot slower. Would you be available to pursue this sort of testing?
  • From a distance, this is a six of one or half a dozen of another change (as far as the code goes), but could we see a container version of each side by side?
    Maybe frameworks and libraries will abstract these differences away with decorators or mixins, but let's surface the requirements that this decentralizes in manual JS. I like the reduction of protection for "bad actors", but it feels like it comes at the addition of "knowing what's right" on the consumer end, but we'll only be able to tell with the code side by side. Take special care to look into the clean-up of removing a consumer from a context, as the possibilities around memory leakage have previously been brought up around the collecting of consumers at the provider level, this would seem to multiply the possibilities of leakage.
  • Further addressing of "open questions"?
    There are a handful of open questions on the current API, where this to support in answering/dispelling them, it would likely go a long way in making this alternative more desired.
  • Extended features
    Could you share a little more on "Surely the consumer would want to handle the case where they won't."? I see this play out in a couple of ways that have yet to get their own issues for discussion, but would appreciate any additional points you would see needing to be addressed as well:
    • a provider is lazily loaded: none is available at the time of request, but one becomes available later on
    • an injected provider: one is available at request time, but then a second further scoped provided is added later on

@mattlucock
Copy link
Author

I have been reading the source of @lit/context and things are making a little bit more sense. It seems to me, especially from @justinfagnani's response, that the protocol as specified is incomplete, and that Lit's custom implementation of the protocol seems to be effectively be the 'full version' of the protocol.

Two notable behaviors in @lit/context:

  • New providers announce themselves and what context they provide to existing providers above them with a context-provider event, and the existing provider can transfer subscriptions to a new provider by faking context requests (which are supposed to come from consumers) on it.
  • Unfulfilled context requests can be caught by an interceptor named ContextRoot which receives notifications from providers using the above mechanism and replays previously made requests to fake that the provider existed at the time the request was made, even though it didn't.

I've noticed this code example in the current protocol document:

class SimpleElement extends HTMLElement {
  connectedCallback() {
    this.dispatchEvent(
      new ContextEvent(
        loggerContext,
        (value, unsubscribe) => {
          // Call the old unsubscribe callback if the unsubscribe call has
          // changed. This probably means we have a new provider.
          if (unsubscribe !== this.loggerUnsubscribe) {
            this.loggerUnsubscribe.?();
          }
          this.logger = value;
          this.loggerUnsubscribe = unsubscribe;
        },
        true // we want this event multiple times (if the logger changes)
      )
    );
  }
  disconnectedCallback() {
    this.loggerUnsubscribe?.();
    this.loggerUnsubscribe = undefined;
    this.logger = undefined;
  }
}

This example closely follows the 'defensive consumer' example and is described as "A more complete example", but in fact it's completely unrelated to the previous example and introduces new ideas not described anywhere else in the document. The protocol does not describe what it means for a consumer to have a 'new' provider; it doesn't specify that a consumer's provider can change, why it would change, how to detect a change, or how to handle a change. It does not specify that the unsubscribe function can dynamically change or that this is a case the consumer needs to be aware of and handle. As I pointed out in the original issue, the protocol does not actually specify that the unsubscribe function needs to be passed to the callback every time, and I think it's completely unintuitive that it does; as a result of these strange semantics, a provider is forced to continually re-provide the same unsubscribe function to a consumer, even though the consumer already has it and does not want it again.

The protocol does not specify any concept of inter-provider communication, yet such a mechanism is integral to Lit's implementation of the protocol, and doesn't specify any mechanism for by which providers can move subscriptions between each other. The protocol doesn't specify any mechanism for faking or replaying requests. The protocol says that requests are dispatched by consumers and caught by providers. This directly lead me to believe that requests are synchronous, but this is, apparently, completely wrong; I'm assuming that it's wrong, but it's not specified by the protocol. The protocol specifies two kinds of participants: consumers and providers. Lit's ContextRoot is effectively a third kind of participant in the protocol, which isn't specified anywhere in the protocol.

I thought this initiative was supposed to be about standardizing the protocol between implementations, but Lit uses what is effectively a custom, non-standard version of the protocol that relies on lots of behavior not specified by the protocol. Again, it seems like this version of the protocol is actually what the protocol is supposed to be, and that the document is missing significant sections. This is so incredibly confusing that this entire issue was essentially borne out of this confusion; it's no wonder my interpretation of the protocol was wrong.

@mattlucock
Copy link
Author

mattlucock commented Nov 18, 2023

I've been reading the source of wc-context, another implementation of the protocol in the wild. In this library, consumers are unaware that providers can 'change' (it's still ambiguous what this means), and don't know that they apparently have the responsibility of detecting this and handling this. The end result of this is that a consumer would end up subscribed to multiple providers simultaneously while only being capable of unsubscribing from the first one.

It's disappointing and frustrating that implementations of the protocol have incompatibilities because what I assume is the most dominant implementation has gone ahead and implemented a lot of non-standard behaviour. It seems clear to me that the current specification of the protocol is inadequate and needs significant changes. I see #50, which is good and necessary, but I do disagree with the framing of "registering learnings"; they've just been making significant changes to the protocol that were never included in the specification. I don't really understand how or why this happened. It is disappointing and frustrating that it's been realised that significant changes need to be made to the specification only now after the specification has been given candidate status. Its current candidate status feels erroneous; it seems to me that the specification is very much still a draft.

@Westbrook
Copy link
Collaborator

Something that might be productive to take into mind before anyone lets frustration or disappointment prevent them from positive action in this area:

  • this is the first protocol to enter “Candidate” status, so we’re all learning about the process as we go. Thanks for being patient and participating productively with the process
  • we currently refrain from making any of the backward compatibility promises that a browser spec does (maybe we should, but that’s a whole different issue), so even if a protocol became “accepted” there’s no reason we couldn’t make breaking changes as new information comes to light
  • along those lines there no reason why a proposal that has reached any level couldn’t be voted down a level if the research that is spurred by any graduation proved that doing so was appropriate. There’s many examples of this in groups like the TC39, etc.
  • learnings from the front should be registered. That doesn’t mean that any one implementation to address them is the “right” approach, but that some implementations will work at different scopes and scale than others and without the learnings that comes from that there is no way to ensure the long term quality of a protocol
  • in the end, these a community protocols, as nice as it would be for each and every implementation to fully implement the patterns outlined here, there’s no contract that could require that beyond them being tested and proved via input from the community like this.

Armed with this information, I hope that you or other interested parties will follow up on the requests above so we can continue to explore this area of functionality with data for the benefit of all!

@mattlucock
Copy link
Author

mattlucock commented Nov 18, 2023

To be clear, I'm happy that the Lit authors identified shortcomings of the protocol and developed solutions to them, but I'm not happy that those solutions were not incorporated back into the protocol. It defeats the point of this whole exercise if implementers rely on non-standard extensions to the protocol, especially if those extensions break compatibility with the existing protocol as specified. @lit/context and wc-context both claim to implement the protocol but are actually incompatible.

My main frustration and confusion here is that it seems like Lit's extensions to the protocol should have and were meant to have been incorporated, but for some reason were not. @justinfagnani identified a number of shortcomings of my proposed protocol, but in fact, all of those shortcomings currently exist in the protocol as specified. I was so confused by his response. It's true that Lit addresses those shortcomings, but it felt like he thought Lit's version of the protocol actually is the protocol as specified. It seems like the there is a fundamental mismatch between the protocol as currently specified and the protocol as imagined and intended. I was, and still am, very confused by this. As I said above, this whole issue was essentially borne out of this confusion and miscommunication.

I don't know the details of how TC39 specifications are developed, but I suppose that was the frame of reference I was using when thinking about these protocols. I'm grateful @Westbrook for you taking the time to talk about these protocols as a concept and what the process for developing them is like. I would suggest that the protocol should be voted down a level, but in saying that I realise we would need to first specify what needs to change and why before we do anything; I guess I'm just thinking ahead. I do think a version of the protocol that effectively incorporates these changes would be substantially different to the current version of the protocol.

I hope I've made my perspective clear. I'm grateful for the responses and the patience. I'll close this.

@gund
Copy link

gund commented Dec 11, 2023

To be honest I actually like the improvements that were suggested here to the event-driven context.

The inversion of the "context-provide" events from providers side back to consumers is really great thing as it will allow to completely eliminate the necessity to store any references about consumers on the provider side whatsoever because providers can use existing EventTarget implementation provided by the DOM itself to handle event subscriptions.
This would also add support for the AbortController as the EventTarget already includes signal property option.

I feel like these changes would cover most if not all of my original concerns about the current state of the protocol in respect of it's API design clarity/usability.

The last concern that is not yet clear in this scenario is how to handle transfer of the consumers from one provider to another here.
But maybe we should ask ourselves do we even want to specify this and make it part of this spec, as maybe this is not that common use-case after all.
Maybe we should define a goals and also non-goals for this protocol and in such a way limit the scope and delegate responsibility of edge-cases to the implementers to be specific.
Or if we really feel like this use-case is very common and must be supported as a first class citizen of this protocol then we should make this requirement clear so that everyone is on the same page and this would greatly align everyone's idea about what this protocol should and should not do.

I'm a bit lost regarding why this issue was closed as it does not seem to me like we've reached any conclusion - did we dismiss the ideas outlined here or did we like them and what to do next?

@mattlucock
Copy link
Author

mattlucock commented Dec 12, 2023

Hi @gund, I appreciate the supportive sentiment.

I think the conclusion from the issue was that my proposal as written is undesirable because it doesn't support out-of-order hydration etc, but that the currently-adopted protocol as written is deficient because it also doesn't support out-of-order hydration, so either way we need to make changes to the protocol. The immediate follow-up to this issue was #50, which is yet to be actioned (but I intend to action it soon; I've just been busy). Even if we don't do any of the changes I proposed, I think there are some issues with the current design of @lit/context which this would still be a good opportunity to address.

Regarding whether this issue is worth incorporating at all: I think I have been persuaded that this is worth caring about. The way I see it, the issue is that the system could easily end up in an invalid state. Let's say A is a parent of B which is a parent of C, and let's say C initially consumes a context from A. Conceptually, a consumer should have the context value from its closest provider, so if B begins providing the context but C does not consume it, C's context value is invalid. I agree that this is an edge case, but I think how this case should be handled probably needs to be specified by the protocol, because if implementers don't handle it in an interoperable manner, this seems to defeat the point of the protocol itself being interoperable. This has already happened, as I mentioned in my previous comment.

All of that said, I think there is a way that my proposal can be adjusted to achieve all of the intended benefits of it while also accommodating this case (I closed this issue simply because it was clear that significant reconsideration was required). In @lit/context, providers announce themselves with a bubbling event when they begin providing. The mechanism would be quite different to the current @lit/context implementation, but I think this event is key. I think the really important question we need to consider is whether we want to pay the perf cost of dispatching events for context value changes; @benjamind has shown that this perf cost is real. I am of the opinion that we probably do want to pay this perf cost since it would greatly simplify the design of the protocol (and also, a provider probably isn't updating a context rapidly anyway). @gund I'm very curious if you have thoughts about this particular question.

@gund
Copy link

gund commented Dec 12, 2023

I see, thanks for elaborating, that makes sense now.

Regarding the cost of an extra event being dispatched during the context update to me it seems like a benefit because of the invention of the control of the subscription (consumer controls vs provider controls) and also it is greatly aligned with the web platform itself semantically as we are not reinventing the wheel for simple stuff that already has been solved and provided by the web platform.
My belief is that the performance cost if at all measurable in real world apps would not be significant enough to make a difference.
Just imagine how often would providers change/update their value. In most real world use-cases this will happen as a one-off setup during an app bootstrap and then periodically between client-side page changes during navigation. So really I think we should first focus on the semantics and usability of the APIs and only when those properties check the box should we concern ourselves about things like performance.

Also another aspect of a good spec is to give enough room for the implementers to do their job, in a sense that could enable them to make optimizations where it makes sense in their implementation and also possibly provide extra features if they so choose.

That is precisely why I actually raised the question about the scope of this spec and maybe we should simply focus on the core functionality where it guarantees only the direct event requests but doesn't guarantee anything more complicated like lazy-context initialization and context transfer between providers.

If the spec would be generic enough and support enough decoupling between consumers and providers these features would be really trivial to implement on top of the base spec, just like lit context is doing currently with their "root context" concept.

Additionally maybe we can break down this spec into multiple levels of sophistication, like having a base spec which only supports direct and just in time context and then additional features like lazy context and context transfers as additional specs that implementers can choose to implement or ignore.
This approach would allow for greater flexibility of the specific implementations depending on the needs, and if for example lazy context spec does not offer enough performance then implementor may choose to not implement it but instead choose a different mechanism for this particular aspect.

This is essentially how I feel about the spec - to make it as decoupled as possible to free up the implementations to be as open as possible while fully interoperable.

But if we really want to go with a fully decoupled consumer/provider then we have to use separate events for context requests and context responses which are dispatched directly to the consumers and not on providers, as this will allow for dynamic provider changes without consumers needing to ever care about it.

@mattlucock
Copy link
Author

mattlucock commented Dec 12, 2023

We're definitely in agreement about the events.

Also another aspect of a good spec is to give enough room for the implementers to do their job, in a sense that could enable them to make optimizations where it makes sense in their implementation and also possibly provide extra features if they so choose.

Again, I think I disagree with this in this context (pun not intended). Implementers can already do, and have already done, whatever they want. The entire purpose of this exercise is to try and standardize a protocol so that varying implementations can interoperate. If implementers extend the protocol in their own ways, then their implementations aren't interoperable–which isn't the end of the world, but seems to defeat the point of the exercise. As I mentioned, using @lit/context and wc-context together right now can actually result in broken behavior.

But if we really want to go with a fully decoupled consumer/provider then we have to use separate events for context requests and context responses which are dispatched directly to the consumers and not on providers, as this will allow for dynamic provider changes without consumers needing to ever care about it.

I was about to say this sounds exactly like #39, but then I realised that you're author of that issue (it all makes sense now). I generally agree with the sentiment from @justinfagnani in that issue; in fact, I actually think your proposed design is too complex.

I understand the general concern about this out-of-order hydration stuff, since it seems like this would be significant source of complexity and 'sophistication' as you put it. Indeed, the current design of @lit/context is quite complex. I think there is actually a very simple mechanism we can use to achieve this. I just need to write it up! I'll be very keen to get your feedback once I do (which I'll try and do sooner rather than later). To be fair, my idea actually involves the consumer being involved in provider changes instead of the provider, which is definitely a break from the current thinking about how this should be done.

@gund
Copy link

gund commented Dec 12, 2023

Just to add a few points on the topic of "more open spec" - I definitely do not mean to have as little in it so it will cause two implementations to not interoperate (like you show example of current state of @lit/context and wc-context).
Essentially the spec must make sure that two completely different implementations of a spec must be able to interoperate but the details on how they achieve this do not necessarily need to be part of the spec.
What I mean by this on real example is if we look at @lit/context they are using something called "root-context" to guarantee lazy-context and probably also context transfers but the spec was not mentioning it, which does not mean they did something wrong but rather extended their implementation further to support something that the spec was not designed for, intentionally or not.

And this is why I think it's important to make coupling between consumers/providers loose enough so that there is room for different implementations which can still easily interoperate without having to deviate from the spec because it's way too strict on the implementation details.
This is what essentially fully event driven context is helping to achieve. And btw your proposal is very similar in that sense to mine as there are two events for context request and context updates.
The only real difference is that you are eliminating the need for a third "unsubscribing" event because of the fact that the event subscription happens on the specific provider instead of on the consumer as in my proposal.

And this is exactly where I think it makes the difference to "open up" the spec to support more sophisticated use-cases like lazy-contexts and context transfers between providers, because when context events are dispatched to the consumers directly - providers are in control and this will allow providers to dynamically change is required and we do not have to specify in the spec how as this very much irrelevant and can be implementation specific but still interoperable because of the way context updates are delivered via events directly to consumers.
Where is in your scenario where context events are dispatched on providers this use-case of context transfers to different providers would not be possible to implement without somehow diverting from the spec because the spec is "locking down" the way events are dispatched by and on specific provider instead of on consumer.

I'm not sure if I was able to convey my thought process on this fully but I hope there is at least some clarity on the reason why I think dispatching context events on consumers instead of providers gives more freedom to implementations and will allow this spec to focus on core things and leaving specifics to implementations while still guaranteeing interoperability.

@mattlucock
Copy link
Author

mattlucock commented Dec 12, 2023

Just in the last few hours or so I've gotten a proof-of-concept implementation of my idea working in, and I think it's quite promising. The entire thing—events, provider, and consumer, all with full support for out-of-order hydration—is ~140 lines.

Two things:

  • I feel like your design delegates more responsibility to providers, whereas I feel that providers are too complex and have too much responsibility.
  • I can totally understand the idea of implementations innovating within a protocol but still remaining interoperable, and I expect the protocol will ultimately allow for such a thing. The concern I have is that there is simply no way to produce provider changes in an interoperable manner within the protocol as currently specified. You seem to be imagining that it can be interoperable without being specified because providers will just sort it out somehow and consumers won't need to know about it—but how would this mechanism work? In this imagined scenario, the providers would need to communicate between themselves, right? How do they communicate? You say that Lit's ContextRoot is an interoperable extension to the protocol, but I don't think this is really true since it relies on a provider communication mechanism that isn't interoperable. It would be great if one implementer's provider could communicate with another implementer's in order to achieve a provider change. Ultimately this comes down to whether you think this is important and relevant to the core behavior of the protocol. I think that it is, and the consensus so far seems to be that it is. If this was going to be a big source of complexity I'd have second thoughts, but I'm very confident it's not going to be.

We'll circle back on this once I've written up my proposal; I'll be very keen to get your feedback. Thank you for your feedback and comments on this.

@gund
Copy link

gund commented Dec 12, 2023

BTW I have a live real project based on lit-html where I already implemented current version of this context protocol and also fully event based approach on top of it as I proposed in the other issue.
So if it's useful we can use it as a test bed to modify the implementation to experiment with the lazy-context and context transfers.

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

5 participants