[base-controller] Fix all any usage, apply universal supertype for functions
#1890
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,38 +1,58 @@ | ||
| export type ActionHandler< | ||
| Action extends ActionConstraint, | ||
| ActionType = Action['type'], | ||
| > = ( | ||
| ...args: ExtractActionParameters<Action, ActionType> | ||
| ) => ExtractActionResponse<Action, ActionType>; | ||
|
|
||
| export type ExtractActionParameters< | ||
| Action extends ActionConstraint, | ||
| T = Action['type'], | ||
| > = Action extends { | ||
| type: T; | ||
| handler: (...args: infer H) => unknown; | ||
| } | ||
| ? H | ||
| : never; | ||
mcmire marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| export type ExtractActionResponse< | ||
| Action extends ActionConstraint, | ||
| T = Action['type'], | ||
| > = Action extends { | ||
| type: T; | ||
| handler: (...args: infer _) => infer H; | ||
| } | ||
| ? H | ||
| : never; | ||
|
|
||
| export type ExtractEventHandler< | ||
| Event extends EventConstraint, | ||
| T = Event['type'], | ||
| > = Event extends { | ||
| type: T; | ||
| payload: infer P; | ||
| } | ||
| ? P extends unknown[] | ||
| ? (...payload: P) => void | ||
| : never | ||
| : never; | ||
|
|
||
| export type ExtractEventPayload< | ||
| Event extends EventConstraint, | ||
| T = Event['type'], | ||
| > = Event extends { | ||
| type: T; | ||
| payload: infer P; | ||
| } | ||
| ? P extends unknown[] | ||
| ? P | ||
| : never | ||
| : never; | ||
|
|
||
| export type GenericEventHandler = (...args: unknown[]) => void; | ||
|
|
||
| export type SelectorFunction<Event extends EventConstraint, ReturnValue> = ( | ||
| ...args: ExtractEventPayload<Event> | ||
| ) => ReturnValue; | ||
|
Comment on lines
+54
to
+55
There was a problem hiding this comment. Highlighting for closer review There was a problem hiding this comment. Good catch, that does seem like a much more appropriate type |
||
| export type SelectorEventHandler<SelectorReturnValue> = ( | ||
| newValue: SelectorReturnValue, | ||
|
|
@@ -41,13 +61,19 @@ export type SelectorEventHandler<SelectorReturnValue> = ( | |
|
|
||
| export type ActionConstraint = { | ||
| type: string; | ||
| handler: ((...args: never) => unknown) | ((...args: never[]) => unknown); | ||
| }; | ||
| export type EventConstraint = { | ||
|
Comment on lines
62
to
+65
There was a problem hiding this comment. So it turns out that The error messages @Gudahtt linked here #1890 (comment) reflect the fact that in general, array types of arbitrary length are not assignable to tuple types with independent typing for some or all of its index positions (the array is wider than the tuple since it has less constraints e.g. the tuple has a minimum length whereas the array could be empty). This means that there's a whole subcategory of functions that To illustrate, In short, I think the top type for functions that includes both of these categories is: ( With this change, |
||
| type: string; | ||
| payload: unknown[]; | ||
| }; | ||
|
|
||
| type EventSubscriptionMap< | ||
| Event extends EventConstraint, | ||
| ReturnValue = unknown, | ||
| > = Map< | ||
| GenericEventHandler | SelectorEventHandler<ReturnValue>, | ||
| SelectorFunction<ExtractEventPayload<Event>, ReturnValue> | undefined | ||
| >; | ||
|
|
||
| /** | ||
|
|
@@ -62,6 +88,9 @@ export type Namespaced<Name extends string, T> = T extends `${Name}:${string}` | |
| ? T | ||
| : never; | ||
|
|
||
| export type NamespacedName<Namespace extends string = string> = | ||
| `${Namespace}:${string}`; | ||
|
|
||
| type NarrowToNamespace<T, Namespace extends string> = T extends { | ||
| type: `${Namespace}:${string}`; | ||
| } | ||
|
|
@@ -151,10 +180,10 @@ export class RestrictedControllerMessenger< | |
| * @param action - The action type. This is a unqiue identifier for this action. | ||
| * @param handler - The action handler. This function gets called when the `call` method is | ||
| * invoked with the given action type. | ||
| * @throws Will throw if an action handler that is not in the current namespace is being registered. | ||
| * @template T - A type union of Action type strings that are namespaced by N. | ||
| */ | ||
MajorLift marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| registerActionHandler<T extends NamespacedName<N>>( | ||
| action: T, | ||
| handler: ActionHandler<Action, T>, | ||
| ) { | ||
|
|
@@ -177,7 +206,7 @@ export class RestrictedControllerMessenger< | |
| * @param action - The action type. This is a unqiue identifier for this action. | ||
| * @template T - A type union of Action type strings that are namespaced by N. | ||
| */ | ||
| unregisterActionHandler<T extends NamespacedName<N>>(action: T) { | ||
| /* istanbul ignore if */ // Branch unreachable with valid types | ||
| if (!action.startsWith(`${this.controllerName}:`)) { | ||
| throw new Error( | ||
|
|
@@ -202,7 +231,7 @@ export class RestrictedControllerMessenger< | |
| * @template T - A type union of allowed Action type strings. | ||
| * @returns The action return value. | ||
| */ | ||
| call<T extends AllowedAction & NamespacedName>( | ||
| action: T, | ||
| ...params: ExtractActionParameters<Action, T> | ||
| ): ExtractActionResponse<Action, T> { | ||
|
|
@@ -227,7 +256,7 @@ export class RestrictedControllerMessenger< | |
| * match the type of this payload. | ||
| * @template E - A type union of Event type strings that are namespaced by N. | ||
| */ | ||
| publish<E extends NamespacedName<N>>( | ||
| event: E, | ||
| ...payload: ExtractEventPayload<Event, E> | ||
| ) { | ||
|
|
@@ -252,7 +281,7 @@ export class RestrictedControllerMessenger< | |
| * match the type of the payload for this event type. | ||
| * @template E - A type union of Event type strings. | ||
| */ | ||
| subscribe<E extends AllowedEvent & NamespacedName>( | ||
| eventType: E, | ||
| handler: ExtractEventHandler<Event, E>, | ||
| ): void; | ||
|
|
@@ -276,13 +305,13 @@ export class RestrictedControllerMessenger< | |
| * @template E - A type union of Event type strings. | ||
| * @template V - The selector return value. | ||
| */ | ||
| subscribe<E extends AllowedEvent & NamespacedName, V>( | ||
| eventType: E, | ||
| handler: SelectorEventHandler<V>, | ||
| selector: SelectorFunction<ExtractEventPayload<Event, E>, V>, | ||
| ): void; | ||
|
|
||
| subscribe<E extends AllowedEvent & NamespacedName, V>( | ||
| event: E, | ||
| handler: ExtractEventHandler<Event, E>, | ||
| selector?: SelectorFunction<ExtractEventPayload<Event, E>, V>, | ||
|
|
@@ -312,7 +341,7 @@ export class RestrictedControllerMessenger< | |
| * @throws Will throw when the given event handler is not registered for this event. | ||
| * @template T - A type union of allowed Event type strings. | ||
| */ | ||
| unsubscribe<E extends AllowedEvent & NamespacedName>( | ||
| event: E, | ||
| handler: ExtractEventHandler<Event, E>, | ||
| ) { | ||
|
|
@@ -335,7 +364,7 @@ export class RestrictedControllerMessenger< | |
| * @param event - The event type. This is a unique identifier for this event. | ||
| * @template E - A type union of Event type strings that are namespaced by N. | ||
| */ | ||
| clearEventSubscriptions<E extends NamespacedName<N>>(event: E) { | ||
| /* istanbul ignore if */ // Branch unreachable with valid types | ||
| if (!event.startsWith(`${this.controllerName}:`)) { | ||
| throw new Error( | ||
|
|
@@ -362,7 +391,10 @@ export class ControllerMessenger< | |
| > { | ||
| private readonly actions = new Map<Action['type'], unknown>(); | ||
|
|
||
| private readonly events = new Map< | ||
| Event['type'], | ||
| EventSubscriptionMap<Event> | ||
| >(); | ||
|
|
||
| /** | ||
| * A cache of selector return values for their respective handlers. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is interesting. This would be a breaking change, and so I am wary of changing this. That said, I don't understand why we chose to use
anyhere. I would be curious to hear if Mark can remember any context around this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ActionConstraintandEventConstraintin this PR are written to be the universal supertypes of their respective categories, so I don't think we'd be adding a meaningful constraint by moving away fromany. Also no tests are being broken by this change, although I guess we'll have to check whether any package outside of the monorepo is using incompatible Action or Event types.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, using
anyin this context is not harmful in the same way that it is in other contexts. Types used in a generic constraints don't get applied to any specific variables, they're just constraints (i.e. by usinganyhere, absolutely nothing is actually given the typeany, even indirectly). For this reason I've sometimes put less effort into avoiding the use ofanyfor generic constraints.That said, more detailed constraints don't hurt either. And in this case, these constraints are already present in the
RestrictedControllerMessengertype so this change seems like it would not affect anything functionally.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generic constraints are also helpful for intellisense hints and self-documenting code. If finding the right constraints isn't a significant blocker for actual feature development, I think it's worth the extra effort, even if it's not strictly necessary in terms of functionality.