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

"issues.labeled" event lacks "payload.label" property type #94

Closed
btorresgil opened this issue Sep 25, 2019 · 5 comments
Closed

"issues.labeled" event lacks "payload.label" property type #94

btorresgil opened this issue Sep 25, 2019 · 5 comments
Labels
Type: Bug Something isn't working as documented, or is being fixed typescript Relevant to TypeScript users only

Comments

@btorresgil
Copy link

This issue depends on octokit/webhooks#43. It is related to #30 .

In the generated webhook schema there is only name, actions, and examples for each webhook. So this library generates the TypeScript types using @gimenete/type-writer from the examples in the schema.

This means that critical payload values that exist in the webhook but are not in the examples are not accessible in TypeScript without some terrible escape hatches.

For example, in an IssuesEvent, a developer trying to access the label payload property would not get prompted that it exists:

2019-09-25_13-24-32

And entering it anyway results in a compiler error, even though the property exists in the webhook:

2019-09-25_13-25-00

The only workaround I've found is to override the types to add the missing properties when they are needed, or escape hatch with any:

webhooks.on('issues.labeled', ({payload}: {payload: any}) => {
  const label = payload.label
})

Since using any removes any type checking, it kinda defeats the purpose of having a schema. So it would be great if the schema could be complete enough to include all the payload parameters in the documentation, and this library could leverage those parameters to build the types.

Thanks!

@btorresgil btorresgil changed the title Webhook types should be generated from Webhook types should be generated from payload, not examples Sep 25, 2019
@gr2m gr2m added the Type: Feature New feature or request label Sep 25, 2019
@oscard0m
Copy link
Member

@btorresgil @gr2m Do you mind if I draft a PR for this once we finish the development here?

@gr2m
Copy link
Contributor

gr2m commented May 5, 2020

The original problem was likely resolved via octokit/webhooks#85, could you verify @btorresgil @dominguezcelada?

I still agree that parsing the payload would be better than the examples. Ideally we would do both in order to find inconsistencies in the documentation, too. Because docs tend to get out of date, while the example payloads are recently recorded.

@gr2m gr2m changed the title Webhook types should be generated from payload, not examples "issues.labelede" event lacks "payload.label" property type May 5, 2020
@gr2m gr2m added Type: Bug Something isn't working as documented, or is being fixed typescript Relevant to TypeScript users only and removed Type: Feature New feature or request labels May 5, 2020
@gr2m gr2m changed the title "issues.labelede" event lacks "payload.label" property type "issues.labeled" event lacks "payload.label" property type May 5, 2020
@oscard0m
Copy link
Member

oscard0m commented May 5, 2020

The original problem was likely resolved via octokit/webhooks#85, could you verify @btorresgil @dominguezcelada?

Yes, now the payload received when issues.labeled occurs contains the label property:

webhooks.js/index.d.ts

Lines 3087 to 3095 in 0f5c95e

type WebhookPayloadIssues = {
action: string;
issue: WebhookPayloadIssuesIssue;
changes?: WebhookPayloadIssuesChanges;
repository: PayloadRepository;
sender: WebhookPayloadIssuesSender;
assignee?: WebhookPayloadIssuesAssignee;
label?: WebhookPayloadIssuesLabel;
};

I still agree that parsing the payload would be better than the examples. Ideally we would do both in order to find inconsistencies in the documentation, too. Because docs tend to get out of date, while the example payloads are recently recorded.

@gr2m Do you want me to create an issue for this (tagging all the related ones) and from there we discuss how to tackle it?

@gr2m
Copy link
Contributor

gr2m commented May 5, 2020

Do you want me to create an issue for this (tagging all the related ones) and from there we discuss how to tackle it?

Sounds good, thanks for your help!

@gr2m
Copy link
Contributor

gr2m commented May 5, 2020

Yes, now the payload received when issues.labeled occurs contains the label property

Thanks for confirming! I'll close the issue as the original problem has been addressed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working as documented, or is being fixed typescript Relevant to TypeScript users only
Projects
None yet
Development

No branches or pull requests

3 participants