refactor(typescript): type State#eventHandler

[jablko]

What do you think about changing State#eventHandler from any to EventHandler? I don't think this affects the external types, just makes the internal types more accurate.

--- a/src/types.ts
+++ b/src/types.ts
@@ -40,7 +41,7 @@ type Hooks = {
 };
 
 export interface State extends Options<any> {
-  eventHandler?: any;
+  eventHandler?: EventHandler;
   hooks: Hooks;
   log: Logger;
 }

[G-Rath]

I think this should remain, since it should be conforming to the State type (which itself is in a bit of a mess)

[jablko]

I'm in favor of dropping it because the inferred type is narrower than State:

• It's assignable to State (the following parameters are State)

  webhooks.js/src/index.ts

  Lines 62 to 63 in dd65cf5

  	this.middleware = middleware.bind(null, state);
  	this.verifyAndReceive = verifyAndReceive.bind(null, state);

• The explicit type discards information: the fact that eventHandler is non-null

That said it's a minor detail, give the word and I'll make this change.

[G-Rath]

I hear what you're saying, and am not wholly against it - however, I always tend to prefer explicitly typing object and arrays in most cases, as it makes it clearer what's in the wrong when there's an error (since it'll error on this assignment being wrong, rather than the usage being wrong), which actually leads me to my second point:

The explicit type discards information: the fact that eventHandler is non-null

Exactly, which may or may not be correct and is effectively the tl;dr of #425 😅

State looks to me like the classic genetically named internal-only state type that tend to popup when you move from JS to TS.

Because of that, I'd prefer to keep the explicit type annotation, to capture what type we're aiming for and hopefully make it easier to spot where our inconsistencies are when refactoring that type.

[wolfy1339]

State looks to me like the classic genetically named internal-only state type that tend to popup when you move from JS to TS.

That is pretty much what happened there when I initially moved to TS.

TBF, we haven't really done much refactoring since, I think it's time to revisit a lot of this stuff with the new experiences we have and new TS features as well

[jablko]

Okay, thanks for this thoughtful explanation. Done ✔️

[G-Rath]

We'd love it! This is part of #425 - the whole State type needs a bit of a cleanup as it's not being used consistently.

You may find that the needs to happen for this change to be landable without having to cast or otherwise modify types elsewhere, but I'd be ok with that if it was only in a few places and wasn't anything too magical.

[wolfy1339]

What needs to happen here in order to get this PR going ahead? @G-Rath @jablko

[gr2m]

I'm okay with doing this change, but we'd need to resolve some conflicts.

I feel like the code is getting a bit messy, but that's okay, we'll clean it up soon (last famous words)

[gr2m]

What needs to happen here in order to get this PR going ahead? @G-Rath @jablko

could one of you let us know what the next is here?

[gr2m]

shall we close this PR?

[wolfy1339]

Yes, lets close this. We can revisit later