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] improve naming of dispose and multiple arguments #24

Closed
benjamind opened this issue Sep 4, 2021 · 1 comment · Fixed by #40
Closed

[context] improve naming of dispose and multiple arguments #24

benjamind opened this issue Sep 4, 2021 · 1 comment · Fixed by #40

Comments

@benjamind
Copy link
Collaborator

Currently the proposal outlines that the context-event should carry a payload that can include the optional argument multiple in order to indicate that the consumer will handle multiple repeated deliveries of the context value. The callback in the payload then receives an optional second argument dispose which is a function which can be invoked by a component to indicate to its provider that it no longer wishes to receive updates.

I'd like to propose that we rename these two parameters to make them more related to each other:

export type ContextCallback<ValueType> = (
  value: ValueType,
  unsubscribe?: () => void
) => void;

export class ContextEvent<T extends ContextKey<unknown>> extends Event {
  public constructor(
    public readonly context: T,
    public readonly callback: ContextCallback<ContextType<T>>,
    public readonly subscribe?: boolean
  ) {
    super('context-request', {bubbles: true, composed: true});
  }
}

So an event is emitted with an argument in its payload to subscribe to further updates to the context value, and then receives in its callback a second argument to unsubscribe from those updates.

This feels to me like it clarifies the intent and behavior here.

@justinfagnani
Copy link
Member

I like subscribe and unsubscribe(). I think it makes it more clear when the callback should be retained by providers, so the answer to #24 is more obvious.

I can make a PR for this.

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

Successfully merging a pull request may close this issue.

2 participants