fix(intellisense): intellisense for array arg in on

[ankeetmaini]

Fix #140

[oscard0m]

Hi @ankeetmaini ! Thanks for contributing to this project. Intellisense is awesome. To make it work 100% for octokit users is a game changer in developer's experience. If we get this fixed will be awesome :)

💬 Feedback:

✅ Tested copy-pasting the index.d.ts in a dummy project and now it shows the events 🎉 :

Before

After

---

❓ With this change I see we lose support for use the same eventHandler for different event types -> i.e.:

this works because these are events from the same "event suite"

webhooks.on(["check_run", "check_run.completed"], ({ name }) => {
    console.log(name, "event received");
})

this does not work because are events from different "event suites"

webhooks.on(["check_run", "label.edited"], ({ name }) => {
    console.log(name, "event received");
})

I think we should keep compatibility for this use case but let's see what @gr2m thinks about it.

---

Extra ball: On this pull request it is planned to cover your use case and split the type declaration in several files to have a better structure. Maybe you are interested on following it up or even participate on it.

[oscard0m]

Also, I would say to wait to land these fixes which will give back stability to index.d.ts. Do you think it would have sense @ankeetmaini

• 🐛 fix(declaration-type): fix missing return type on createWebhooksApi #137
• 💚 ci(test): add typescript validation as a step in Test workflow in CI #139

[ankeetmaini]

Also, I would say to wait to land these fixes which will give back stability to index.d.ts. Do you think it would have sense @ankeetmaini

• 🐛 fix(declaration-type): fix missing return type on createWebhooksApi #137
• 💚 ci(test): add typescript validation as a step in Test workflow in CI #139

Sure, it can wait till we get these two merged in!

[ankeetmaini]

I can try and see if we can keep the same handler usecase. Also if #113 is slated to go in soon, I can close this and help out. :)

[oscard0m]

I can try and see if we can keep the same handler usecase. Also if #113 is slated to go in soon, I can close this and help out. :)

🆒 , if you need help with it let us know :)

[ankeetmaini]

@dominguezcelada I've removed all the hardcoded overloads and used conditional types to store the mapping of type -> Payload. I also added the same signature for removeListener which now adds intellisense to it.

Intellisense works great for array params (likewise for on)

it calculates the payload types

[gr2m]

I think we should keep compatibility for this use case but let's see what @gr2m thinks about it.

I agree but this is better than what we have now.

@dominguezcelada PR looks good to me if you think it's good. We can iterate on details

[oscard0m]

@ankeetmaini nice job doing the type generation more generic with conditional types.

PR's fixing index.d.ts are on master already.

An idea that came in my mind is to extend typescript-validate.ts test adding the following cases to double check your changes are keeping retrocompatibility:

webhooks.on("check_run.completed", () => {})
webhooks.on(["check_run.comppleted", "check_run.created"], () => {})
webhooks.on(["check_run.comppleted", "commit_comment"], () => {})

And for
webhooks.removeListener()

If you need help with it, let me know :)

[ankeetmaini]

@dominguezcelada I've added the changes for validating type fixes. Let me know if something more needs to be done!

[oscard0m]

@dominguezcelada I've added the changes for validating type fixes. Let me know if something more needs to be done!

Amazing Job @ankeetmaini. Tried it in a dummy repo and works good to me :)

[github-actions]

🎉 This PR is included in version 7.6.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀