Closed Keyrxng closed 5 months ago
I am skeptical here because the whole team tried and nobody could figure it out.
Believe me so was I for this very same reason but I went through the files looking for other ts errors and couldn't find any. Checked the tsserver
logged types, used twoslash
etc and confirmed the output types look the same as they did before.
This is building a mapped type which is basically an object using the keys of the WebhookEventName
union type and assigning the underlying BaseWebhookEvent
as the values of those keys. We know that K
will always extend WebhookEventName
, so we are basically removing the conditionals from the Octokit types.
export type SupportedEvents = {
[K in SupportedEventsU]: K extends WebhookEventName ? WebhookEvent<K> : never;
};
This is saying "context takes a key, any key of the union type. Use that key to index the type object which will return only the webhook event for that key"
T
remains the string to index by and TU
becomes a single object using that key by indexing a type which is straightforward.
GitHubContext<T extends SupportedEventsU = SupportedEventsU, TU extends SupportedEvents[T] = SupportedEvents[T]>
TU["payload"]
can only be the payload assignable to that specific key as TU
is only possible through T
TU["payload"];
If I show you the difference between the two types as the tsserver
sees it.
Original
"(alias) getDefaultBranch(
context: GitHubContext<\
"branch_protection_configuration\" | \"branch_protection_rule.disabled\"
| \"branch_protection_rule.enabled\" | \"branch_protection_rule\"
| \"branch_protection_rule.created\" | ... 270 more ...
| \"workflow_run.requested\">, owner: string, repository: string): Promise<...>\n
import getDefaultBranch"
New
"(alias) getDefaultBranch(
context: GitHubContext<\
"branch_protection_configuration\" | \"branch_protection_rule.disabled\"
| \"branch_protection_rule.enabled\" | \"branch_protection_rule\"
| \"branch_protection_rule.created\" | ... 270 more ...
| \"workflow_run.requested\", BaseWebhookEvent<...> | ... 274 more ...
| (BaseWebhookEvent<...> & {\n ...;\n})>, owner: string, repository: string): Promise<...>\n
import getDefaultBranch"
You see here that despite it actually being more verbose and includes another whole type, it's less work for TS because we've constrained things, so rather than TS trying to produce options through inference for all possible combinations of the union type when saying WebhookEvent<T>["payload"]
it's now only having to index a typed object with T
that'll return the event or never
, there is no inference involved for it beyond identifying which key of the union it is.
This is how I understand the problem and how it looks in my head
I didn't really touch on why I think this resolves the underlying issue
We are abstracting inference logic into types which are basically TS functions. So rather than TS trying to do some wizard shit, the path for it to find the underlying webhook event is cut short because we are building a Record
of EventName:Event
which is more efficient than the Octokit type which has many conditionals and lots of variances in payload etc.
So I think it's just a more direct and efficient path to the underlying type taking some of the burden off of the TS server that allows it to infer easier or that it's due to the generic TU
being explicitly defined which is carrying some of the load.
This is just my opinion tho I don't have anything to support this really beyond what I've shared.
Using the below approach throws this error, which is pretty much how it was being used before with WebhookEvent<T>["payload"]
and I expect that is because WebhookEvent wants a specific key of the union and it's being passed a type which is the whole union which is just a guess but logically you'd expect this to work looking at it.
GitHubContext<T extends WebhookEventName = WebhookEventName, TU extends WebhookEvent[T] = WebhookEvent[T]>
Type 'T' cannot be used to index type 'EmitterWebhookEvent'.ts(2536)
This looks promising, I tried it and it works. I can also see why it's easier for TS to resolve a mapped type instead of trying to infer so many combinations.
I've used your example and removed TU
so we only need to provide one type and it looks it's working
export class GitHubContext<TSupportedEvents extends WebhookEventName = WebhookEventName> {
public key: WebhookEventName;
public name: WebhookEventName;
public id: string;
public payload: {
[K in TSupportedEvents]: K extends WebhookEventName ? WebhookEvent<K> : never;
}[TSupportedEvents]["payload"];
public octokit: InstanceType<typeof customOctokit>;
public eventHandler: InstanceType<typeof GitHubEventHandler>;
@Keyrxng what do you think?
A much cleaner way to represent it for sure @whilefoo
I wasn't sure I knew the various edge cases to try that you'll have worked around or whatever but I'm happy it's working
@whilefoo I'm unsure whether to make those changes or if you are going to PR/commit against this or just close as completed and use the new approach, lmk if there is something I need to do.
@Keyrxng you can make the change
I think you commented in the same sec that I pushed the change 😂
Resolves #31
Using the same type setup I've used in https://github.com/ubiquibot/plugin-template
https://github.com/ubiquity/ubiquibot-kernel/assets/106303466/e04d59f7-e720-439a-874a-84440138288b