feat: allow webhookCallback to infer adapter types

[winstxnhdw]

Background

Traditionally, webhookCallback has never been strictly typed. It returns a handler that takes in arguments of any[] and returns any. This can be confusing to new users, forcing many attempts at different configurations before webhookCallback is able to work correctly.

Objective

This PR will overload webhookCallback with typings for all adapters, allowing TypeScript to resolve the appropriate handler parameters and return types. We also add tests to track any incompatibility between our supported web frameworks and the respective adapter implementation.

With the newly resolved types introduced with structural typing, our intellisense can now emit an error when an incorrect configuration is entered.

[winstxnhdw]

I am not exactly satisfied with the current implementation. I understand that we want to reduce dependencies, so here's a compromise: Let's write incomplete type stubs for each adapter.

Here's an example.. Although this hono adapter consumes a ctx, we can write a contract for only the properties we need.

const hono: FrameworkAdapter = (ctx) => {
    let resolveResponse: (res: Response) => void;
    return {
        update: ctx.req.json(),
        header: ctx.req.header(SECRET_HEADER),
        end: () => {
            resolveResponse(ctx.body());
        },
        respond: (json) => {
            resolveResponse(ctx.json(json));
        },
        unauthorized: () => {
            ctx.status(401);
            ctx.statusText(WRONG_TOKEN_ERROR);
            resolveResponse(ctx.body());
        },
        handlerReturn: new Promise<Response>((resolve) => {
            resolveResponse = resolve;
        }),
    };
};

In this case, we only need req, req.json, req.headers, body, json, status and statusText. So we can write an interface like the following:

export interface HonoContext {
    req: {
        json: () => Promise<Update>;
        header: (header: string) => string | undefined;
    };
    body: () => Response;
    status: (status: number) => void;
    statusText: (statusText: string) => void;
    json: (json: string) => Response;
}

Then our webhookCallback can be:

export function webhookCallback<C extends Context = Context>(
    bot: Bot<C>,
    adapter?: "hono",
    webhookOptions?: WebhookOptions,
): (request: HonoContext) => Promise<Response>;

This is a good idea because we already use the following properties in our implementation: req, req.json, req.headers, body, json, status and statusText. It is not a bad thing to have them typed. If there are any breaking changes on the adapter side, we would have to fix it on our end regardless of whether they are typed or not.

In the long run, there may be the added benefit of easier maintenance. Do let me know what you think.

[KnorpelSenf]

That is what I suggested in https://t.me/grammyjs/235629. We could have done this from the inception of the library, but I wanted to move fast back then and couldn't be bothered to figure out the correct types for each adapter. I focused on adding more adapters instead. Since the concept of framework adapters is mature now, we can start adding the types exactly like you suggested, so if you feel like spending time on getting these things done, I'd be more than happy to merge such changes.

The approach outlined above will even work for all adapters, not just the ones with globally available types.

[winstxnhdw]

I wonder if we still need this method here. Seems a bit redundant.

export function webhookCallback<C extends Context = Context>(
    bot: Bot<C>,
    adapter?: SupportedFrameworks | FrameworkAdapter,
    onTimeout?: WebhookOptions["onTimeout"],
    timeoutMilliseconds?: WebhookOptions["timeoutMilliseconds"],
    secretToken?: WebhookOptions["secretToken"],
): (...args: any[]) => any;

[KnorpelSenf]

It is only there for backward compatibility and will be removed for the next major version. It used to be the only overload.

[winstxnhdw]

Maybe we should mark as deprecated?

[KnorpelSenf]

Sure, if you want :)

[winstxnhdw]

I’d like to suggest that we have a simple regression ‘test’ (just against the types) for every adapter. This way, we can ensure that our adapters are always compatible and only have to keep the framework types as devDependencies. What do you think?

[KnorpelSenf]

Amazing plan. Another thing that I was just too lazy to do so far.

@PonomareVlad you're gonna like to see this PQ

[KnorpelSenf]

The next minor version will be released now (without these changes) but I am happy to follow up with a patch release as soon as this is done :)

[winstxnhdw]

@winstxnhdw I fixed up the permission errors, please TAL at the commit message

This is great, I wish I thought of this earlier. However, I had purposefully exposed the parameters for easier maintenance. If there's a breaking change in the future, it can be difficult to identify what's the offending property.

[KnorpelSenf]

the parameters

Which ones? I can't quite follow rn

[winstxnhdw]

Which ones? I can't quite follow rn

For example, instead of

    it("Express should be compatible with grammY adapter", () => {
        const app = { post: () => {} } as unknown as Express;
        app.post("/", webhookCallback(bot, "express"));
    });

Maybe it's better to do

    it("Express should be compatible with grammY adapter", () => {
        const app = { post: () => {} } as unknown as Express;
        app.post("/", (req, res) => { 
          return webhookCallback(bot, "express")(req, res) 
        });
    });

This way the intellisense can help us rather than relying on the verbose error messages alone.

[KnorpelSenf]

Oh. I see. That is not how I would approach debugging that, so I did not even think of that. I mostly inlined it to illustrate idiomatic usage of webhookCallback to whoever may read the tests as documentation (and because it is shorter).

But I also don't have too strong opinions about it, so if you feel like the code is more maintainable, we can just as well turn the things into lambdas again. Would you like me to revert this part of the changes?

[winstxnhdw]

Oh, how else would you be debugging this? If there's a better way that is not reading the error messages, we can stick with this. Otherwise, I think it's best to turn them back into lambdas again.

[KnorpelSenf]

If there's a better way that is not reading the error messages

no, I will always read error messages, but that does not mean that they have to be my primary way of drilling down on the problem :D

I usually read the type definitions and changelogs in these cases, that is enough to know the problem. If it isn't, I usually write a few lines of code to match the specific parts exactly.

Either way, I'll revert those changes. People work differently and I want things to be simple as possible for everybody.

[KnorpelSenf]

@winstxnhdw would you like to perform https://t.me/grammyjs/237669 in the same PQ, or rather get this merged and follow up with more changes?

[winstxnhdw]

It's a bit out of the scope of this PR. I think that should be in another PR. Let's get this one merged first.

[KnorpelSenf]

Alright, agreed. Regarding aae3884, I am no longer comfortable with option 1. We may do option 2 or also just leave it and merge this as-is. What do you prefer?

[winstxnhdw]

I want to try and keep our PRs cleaner and with a single objective for future reference. Let’s leave that for the next PR. Also, I am not exactly sure how we want to test the runtime. We should discuss this more in the chats.

[KnorpelSenf]

This PQ already introduces runtime tests for some of the adapters, but none of the others. But we've iterated long enough on this one, I'm more than happy to merge.

[KnorpelSenf]

Please consider signing your commits in the future :)

[KnorpelSenf]

@all-contributors add @winstxnhdw for idea and code and review and test

[allcontributors]

@KnorpelSenf

I've put up a pull request to add @winstxnhdw! 🎉