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

Allow for custom events in receive() without warning #277

Open
bcoe opened this issue Sep 23, 2020 · 4 comments
Open

Allow for custom events in receive() without warning #277

bcoe opened this issue Sep 23, 2020 · 4 comments
Labels
Status: Blocked Blocked by GitHub's API or other external factors Type: Support Any questions, information, or general needs around the SDK or GitHub APIs

Comments

@bcoe
Copy link

bcoe commented Sep 23, 2020

Problem

We've gotten into the habit of using a few custom events in our bot infrastructure (you can see the approach in action here).

The latest version of probot is stricter about the events accepted, so we ended up needing to do something like this:

// eslint-disable-next-line @typescript-eslint/no-explicit-any
name: 'foo' as any,

I believe this approach works, but we receive a warning.

Proposed Solution

It would be great to have another method, e.g., receiveLoose, or receiveUnchecked, which:

  • accepted a string rather than a EventNames.StringNames.
  • did not warn.

Beyond folks like us, who've written some of our own custom events, I believe this would create value for folks who are pinned on older versions of the library, when/if new event types are added to the GitHub API.

@gr2m gr2m added the Type: Support Any questions, information, or general needs around the SDK or GitHub APIs label Sep 23, 2020
@gr2m gr2m self-assigned this Sep 23, 2020
@gr2m
Copy link
Contributor

gr2m commented Sep 25, 2020

If possible, I'd like to avoid adding a new API such as .receiveLoose or .receiveUnchecked. I'd prefer to allow users to register custom event types in some way. Ideally, the registration would be part of the Webhooks constructor, and it would allow to register

  1. The event name
  2. The types for the event payload.

I'm not sure how that would possibly work. 1. is easy, could be

const webhooks = new Webhooks({
    customEvents: ['custom1','custom2']
})

In terms of TypeScript, I'm not so sure. I guess we could add a type variable

type CustomEvent1 = { name: "custom1", payload: { /* ... */ } }
type CustomEvent2 = { name: "custom2", payload: { /* ... */ } }
type CustomEvents = CustomEvent1 | CustomEvent2
const webhooks = new Webhooks<CustomEvents> ({
    customEvents: ['custom1','custom2']
})

But I wonder if there is a more elegant solution.

Somewhat related, I've been thinking about custom events for a while myself. Less in terms of being able to receive entirely new type of events, and more as a plugin mechanism, where I could register say a pro-plan:pull_request event, which would let me subscribe to a pull_request event, but only when the installation has purchased a pro plan in the marketplace. That could work similar to Octokit's plugin API with a new "event" hook (or similar).

I at least want to think about it so we don't introduce APIs today that would make our live harder tomorrow

Update: nevermind the event plugin thinking. It's quite different and will likely happen on a different level.

I believe this would create value for folks who are pinned on older versions of the library, when/if new event types are added to the GitHub API.

So far that was not a problem, we do have automated updates in place, and we could expand it to all recent breaking versions, so even people stuck in v5, v6, etc could still get all supported event names out of the box.

@gr2m
Copy link
Contributor

gr2m commented Sep 25, 2020

@bcoe what do you think about this API

type CustomEvent = {
  name: "custom";
  id: string;
  payload: {
    /* types for your possible payloads */
  };
};
const webhooks = new Webhooks<CustomEvent>({
  customEvents: ["custom"],
});

webhooks.on("custom", (event) => {
  // get full type testing and intellisense for `event`
});

No idea how exactly this would work, but I think it could :D

@bcoe
Copy link
Author

bcoe commented Sep 30, 2020

@gr2m sorry for the slow response. I think something like this would be great, could we potentially provide it to the upstream API via a plugin, then perhaps I could configure it with the octokit instance.

@gr2m
Copy link
Contributor

gr2m commented Oct 1, 2020

I think there is a better approach that doesn't require a type argument, see my comment here: #250 (comment)

@gr2m gr2m removed their assignment Mar 17, 2021
@gr2m gr2m added the Status: Blocked Blocked by GitHub's API or other external factors label Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Blocked Blocked by GitHub's API or other external factors Type: Support Any questions, information, or general needs around the SDK or GitHub APIs
Projects
None yet
Development

No branches or pull requests

2 participants