fix: type the return of createEventHandler

[G-Rath]

This fixes the error that occurs when building the project currently, caused by merging #406

Error: src/event-handler/index.ts(10,17): error TS4058: Return type of exported function has or is using name 'BaseWebhookEvent' from external module "/home/runner/work/webhooks.js/webhooks.js/src/types" but cannot be named.

This occurs because createEventHandler doesn't have an explicit return type but is exported meaning TypeScript has to infer the return for the declaration file. For efficiently when doing this TypeScript inlines types as much as possible leading to the above error because it infers the type of the onAny property of the return to be handler: (event: import("../types").BaseWebhookEvent<"*">) => any) => void, which errors since BaseWebhookEvent is not exported.

This actually revealed it's also doing this for EmitterEventName, meaning the declaration file for createEventHandler has an inline string union of ~200 strings four times (because we're doing EmitterEventName | EmitterEventName[] so it's doing "push" | "error" | "public" | "label" | "meta" | ... | ( "push" | "error" | "public" | "label" | "meta" | ...)[]).

So I've explicitly defined the return type for createEventEmitter - this type ideally should be propertaged to the rest of the project (i.e State#eventEmitter should be using this type), but it's not an easy thing to apply for a few reasons, so will leave that for a follow-up PR down the line (tbh would be good if we could remove/reduce State as much as possible, as it's a complex object)

[oscard0m]

Some questions coming up to me @G-Rath

• What's the impact of this error? Which problems could potentially suffer end users?
• Is this a temporary solution for remove friction with end users? From your explanation seems like we should invest some more time on better typing this (?). If so, would have more sense reverting feat: use new types from @octokit/webhooks-definitions #406 and tackling this properly?
• Is it possible (maybe in a different PR) to add test coverage to prevent this breaking change in the future?

[oscard0m]

❓ Why TTransformed for naming?

[oscard0m]

Should this be

Suggested change

	interface EventHandler<TTransformed = unknown> {
	interface EventHandler<TTransformed = {}> {

Considering U = {} here?

webhooks.js/src/index.ts

Line 24 in d3df9f3

class Webhooks<T extends EmitterWebhookEvent = EmitterWebhookEvent, U = {}> {

[G-Rath]

This is the name I used for the generic in HandlerFunction - ideally I'd rename U to match, but held off doing that for now to reduce the scope of the changes in both this PR & in #406 .

The reasoning for the name is that it gives you more information about what the generic actually represents, which is good as generics tend to exist in a vacuum so it's not as easy to know what values should be being passed in when using the type (i.e "should this be the event payload, or the result of what my custom transformer function is doing?").

I'm not against single letter generics, but try to only use them when they're obvious i.e

removeListener<E extends EmitterEventName>(
    event: E | E[],
    callback: HandlerFunction<E, TTransformed>
  ): void;

Because E has a constraint, it's far more obvious that it's the name of an event, but if TTransformed was just U you'd not have a lot of information on what it's actually representing (as the extends doesn't clear things up either).

Considering U = {} here?

{} is actually means "any non-nullish value", so it's not safe to use. unknown is the better type as we've got no constraints on what type can be passed, meaning a user can transform the value into anything (including nullish values).

I believe that U = {} should be changed to use unknown as well, but I've not done it because of scoping + it's one of those things that while I know of the gotcha I can't remember all the rules around it well enough to do the replacement without playing around with it first (which I need the time to do) :)

[oscard0m]

I'm a big fan of your explanations. Thanks for sharing your knowledge and taking your time on giving explanations in detail.

[oscard0m]

Should this be equivalent to this?

webhooks.js/src/index.ts

Lines 37 to 41 in d3df9f3

	public receive: (options: {
	id: string;
	name: string;
	payload: any;
	}) => Promise<void>;

[G-Rath]

Actually it should be the other way around - I've deliberately not modified that type because I don't want to blow out the scope of the PR more than needed, and the types are compatible so it's not a required change.

[oscard0m]

Actually it should be the other way around - I've deliberately not modified that type because I don't want to blow out the scope of the PR more than needed, and the types are compatible so it's not a required change.
👍🏽

I will create an issue + PR for this if you are ok with it :)

[G-Rath]

I will create an issue + PR for this if you are ok with it :)

Go for gold! There's a number of potential improvements to the types and structure that I think can be made but have held off doing until #406 was landed as they're about smoothing out the internal types rather than external, and so would have been moot to land before a big refactor to the types.

That's also why I aimed for functionality first in that PR, rather than ensuring the types were completely perfect (i.e making sure there were no type aliases for the same type)

[G-Rath]

What's the impact of this error? Which problems could potentially suffer end users?

This is preventing us from building the project, so we can't do any releases - so you could say the impact for the end user is nothing (since they're not pulling any new code), or everything (since we can't release any new code for fixs, features etc) :)

Is this a temporary solution for remove friction with end users?

No, this is the fix that'll allow us to properly ship #406 - while the smallest version of this fix is to just export BaseWebhookEvent, that is less obvious (since the export wouldn't be used in src, so would look unneeded unless you inspected the built types), and this is the better solution anyway as we're explicitly defining what the function is expected to return by those using it.

From your explanation seems like we should invest some more time on better typing this

The types for this PR are correct it's our internal types that could use improvement, and definitely worth investing improving. However the gains vs amount of work that'd be involved would be pretty low so I'd not say it's a super high priority (in particular because of the State type - i.e we mark eventEmitter as optional, but don't actually check for its existence in a few places; our implementation + test suite ensure it's not undefined at runtime, but the types are not representing that properly).

Is it possible (maybe in a different PR) to add test coverage to prevent this breaking change in the future?

Totally - so a quick solution would be to just do npm run build as part of our PR checks, to ensure building is always successful, but we can also use @typescript-eslint/explicit-module-boundary-types to ensure all exports are completely typed.

That's something I think should be addressed in another PR.

[oscard0m]

What's the impact of this error? Which problems could potentially suffer end users?

This is preventing us from building the project, so we can't do any releases - so you could say the impact for the end user is nothing (since they're not pulling any new code), or everything (since we can't release any new code for fixs, features etc) :)

👍🏽

Is this a temporary solution for remove friction with end users?

No, this is the fix that'll allow us to properly ship #406 - while the smallest version of this fix is to just export BaseWebhookEvent, that is less obvious (since the export wouldn't be used in src, so would look unneeded unless you inspected the built types), and this is the better solution anyway as we're explicitly defining what the function is expected to return by those using it.

👍🏽

[oscard0m]

From your explanation seems like we should invest some more time on better typing this

The types for this PR are correct it's our internal types that could use improvement, and definitely worth investing improving. However, the gains vs amount of work that'd be involved would be pretty low, so I'd not say it's a super high priority (in particular because of the State type - i.e. we mark eventEmitter as optional, but don't actually check for its existence in a few places; our implementation + test suite ensure it's not undefined at runtime, but the types are not representing that properly).

We can create an issue of it (maintenance label) so at least we keep track of it. Thumbs Up if you are ok with it.

[oscard0m]

Is it possible (maybe in a different PR) to add test coverage to prevent this breaking change in the future?

Totally - so a quick solution would be to just do npm run build as part of our PR checks, to ensure building is always successful, but we can also use @typescript-eslint/explicit-module-boundary-types to ensure all exports are completely typed.

That's something I think should be addressed in another PR.

I can create an issue for this too, and we discuss if npm run build as a Check Run would be the solution to go. Thumbs up if you are ok with it

[github-actions]

🎉 This PR is included in version 7.22.0 🎉

The release is available on:

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

Your semantic-release bot 📦🚀