🚧 fix(typescript): remove new Webhooks type arguments

[gr2m]

This is a follow up to #292 (comment)

I really do not like that we introduced this type argument as I think there should be a way to infer it from the transform option if present.

This should also address

it looks like the types think that octokit is no longer a property on the context passed into a webhook:

from #436 (comment)

In a nutshell, this should work

const webhooks = new Webhooks({
  secret: "bleh",
  transform: (event) => {
    return {...event, foo: "bar" };
  },
});
webhooks.on("issues", (event) => {
  // event.foo is typed as "string"
  console.log(event.foo);
});

Anyone up for looking into this?

[G-Rath]

Removing these are a really bad idea:

• TypeScript cannot infer without generics, so removing them will just mean you'll always have to cast to represent the action of transforming
• There's no way for TS to infer from transform, due to the way it handles inference (I don't know the exact rules for this situation, but it can't handle inferring the value of TTransformed from the return of a function of a property).

Actually, so TS can infer the value of TTransformed, it just can't handle apparently the error deprecated syntax, likely because it can't handle the inference of E.

However, either way the generic has to stay in order for the inference to work.

[gr2m]

However, either way the generic has to stay in order for the inference to work.

Yes, that's how it worked before, too. Sorry, I should have been more clear.

What I'd like is to remove the requirement to set TTransformed, it should instead be inferred by default.

Would that work?

[G-Rath]

There isn't a requirement to set it - that's why it's got a default value; if TypeScript can infer the value, it will, otherwise it'll use the default value.

[gr2m]

But it currently is required to be set if I set options.transform which returns an object different from the standard event object, that's why the tests currently fail. What I want is the tests with my changes to pass. It worked before #292

[G-Rath]

Yeah, that's because we need a new generic argument being passed to Options, but then that'll break on the deprecated error usage.

Give me a sec and I'll push up the required change

[G-Rath]

(btw this change seems to cause a massive spike in the amount of work TS/my IDE has to do :()

[gr2m]

I gotta run now. If I recall correctly, the way it worked was that the type passed to the .on() handler callback was set to the return / resolve type of options.transform. And by default options.transform just did (event) => event. If I changed transform to a custom method using the transform constructor option such as (event) => {...event, foo: 'bar'}, then event.foo would be correctly typed, without me needing to pass any type parameters. It just worked

[G-Rath]

Yup, and that's how it works now too - but that can't work for the "error" event due to the nature of the types.

This shouldn't be a problem because we're removing "error" anyway.

[gr2m]

Got it, so we can resolve this as part of the breaking changes of the v8.0.0 release?

[github-actions]

🎉 This issue has been resolved in version 8.0.3 🎉

The release is available on:

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

Your semantic-release bot 📦🚀