fix: typescript autocomplete and types for public api #292
Conversation
gr2m
added
Type: Bug
src/generated/event-names.ts
Outdated
| | WatchEvent | ||
| | WorkflowDispatchEvent | ||
| | WorkflowRunEvent; | ||
| type All = keyof EventTypesPayload; | ||
| } |
There was a problem hiding this comment.
can we get rid of this file altogether?
There was a problem hiding this comment.
Done! I only did not remove it as it was exported from entrypoint. Will this be a breaking change for the users?
|
|
||
| class Webhooks<T extends WebhookEvent = WebhookEvent> { | ||
| class Webhooks<T extends WebhookEvent = WebhookEvent, U = {}> { |
There was a problem hiding this comment.
U represents the additional properties that the transform function returns apart from the event payload object. Used here https://github.com/ankeetmaini/webhooks.js/blob/4e8d65b891f4adaa1426eb505a92b321c307bb2f/test/typescript-validate.ts#L124
There was a problem hiding this comment.
I think before the type was inferred automatically from the return type of the transform method, is that still the case?
There was a problem hiding this comment.
I'm not sure how that can be achieved. I can try and move the Option<T> param up so that it can be passed down to on handler and then try and extract the return type of Option<T>['transform'] but since the transform function would return an intersection type like WebhookEvent<any> & {some: 'other', type: 'attributes'} I can't use a conditional value to extract the second part. Mostly because intersection types are not distributed like union ones.
Also if I take the entire value, then intellisense goes out the window as WebhookEvent<any> takes precedence.
There was a problem hiding this comment.
This can probably be solved by changing the Webhook<T> signature, right now T is the actual payload, if we're to map that as the event name, then we can also theoretically get the value returned automatically. But this would be a big change like I was trying to attempt in #258 and I was all over the place :P
There was a problem hiding this comment.
Okay, let's leave it as is, we can do a follow up PR if we come up with something clever :)
Can you please add a comment explaining the U type parameter?
There was a problem hiding this comment.
I've added the comment! I'll try and see if I can improve the types further in a follow up PR :D
There was a problem hiding this comment.
Changes look good to me, and I confirmed locally that #279 is resolved by this PR
@dominguezcelada @wolfy1339 can one of you have another look?
|
Seems to work fine for me |
|
🎉 This PR is included in version 7.12.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Sorry for the delay on following up this. Even it's already merged. Looks good to me 👍🏽 @ankeetmaini amazing effort and job here. Congrats! :) |
Improved with octokit/webhooks.js#292
It also uses the approach defined in #250 to use interface types instead of a big conditional.
Fixes #250
Fixes #279