grammyjs / grammY

The Telegram Bot Framework.
https://grammy.dev
MIT License
2.04k stars 106 forks source link

feat: allow `webhookCallback` to infer adapter types #565

Closed winstxnhdw closed 2 months ago

winstxnhdw commented 2 months ago

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 commented 2 months ago

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.

winstxnhdw commented 2 months ago

Looks like Deno.requestEvent can't be backported :(

KnorpelSenf commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

winstxnhdw commented 2 months ago

Maybe we should mark as deprecated?

KnorpelSenf commented 2 months ago

Sure, if you want :)

winstxnhdw commented 2 months ago

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 commented 2 months ago

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

@PonomareVlad you're gonna like to see this PQ

KnorpelSenf commented 2 months ago

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 commented 2 months ago

Thanks! Will get this done soon. Just need to figure out how we can test TypeScript in Deno by compile-time.

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 0% with 55 lines in your changes are missing coverage. Please review.

Project coverage is 45.66%. Comparing base (c1c7d8c) to head (14020bf). Report is 8 commits behind head on main.

:exclamation: Current head 14020bf differs from pull request most recent head a51f27a. Consider uploading reports for the commit a51f27a to get more accurate results

Files Patch % Lines
src/convenience/frameworks.ts 0.00% 54 Missing :warning:
src/convenience/webhook.ts 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #565 +/- ## ========================================== - Coverage 45.80% 45.66% -0.15% ========================================== Files 19 19 Lines 5148 5166 +18 Branches 335 335 ========================================== + Hits 2358 2359 +1 - Misses 2786 2803 +17 Partials 4 4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

winstxnhdw commented 2 months ago

It seems that the implementation for Azure function's callback is not done correctly and I am not sure how to add the types for Express to Deno since they live in @types/express

KnorpelSenf commented 2 months ago

It seems that the implementation for Azure function's callback is not done correctly

That is entirely possible, it isn't very popular so feedback is very rare.

I am not sure how to add the types for Express to Deno since they live in @types/express

Wouldn't this be done the same way as with all the other adapters, i.e. by defining a minimal interface in the types and doing it in a way that is compatible with express? Why would it be a problem that express is on npm?

winstxnhdw commented 2 months ago

Wouldn't this be done the same way as with all the other adapters, i.e. by defining a minimal interface in the types and doing it in a way that is compatible with express? Why would it be a problem that express is on npm?

Because npm:express doesn't come with types. I am not sure how Deno can resolve @types/express. Do I just do this?

import * from 'npm:@types/express'
KnorpelSenf commented 2 months ago

I see. No, you have to add a comment. See https://docs.deno.com/runtime/manual/node/npm_specifiers#typescript-types.

You can also just test it on node.

winstxnhdw commented 2 months ago

You can also just test it on node.

I want to try to keep everything on Deno as much as possible.

That is entirely possible, it isn't very popular so feedback is very rare.

It seems that it isn't entirely incorrect. Azure functions has received a major version update to v4.0. The adapter implementation seems to be for v3.0. However, the implementation seems off as I can't find the send method anywhere. For the sake of getting this PR out quickly, I'll skip testing Azure functions for now.

winstxnhdw commented 2 months ago

I have added some Deno permission flags when running the tests. This is because try-catch doesn't work on Node modules. Not sure if this will be a security risk. Do check.

winstxnhdw commented 2 months ago

I am unsure how to test sveltekit's. As for worktop, I think it's best if we wait for Deno compatibility, which is coming very soon.

KnorpelSenf commented 2 months ago

Do your tests actually cover the functionality somehow, asking because it looks to me like they're just there for type-checks?

Also, do you think we should pin the versions of our dependencies in the tests? I see good reasons for and against doing that.

winstxnhdw commented 2 months ago

They are there for type-checking only. I didn’t pin the dependencies because I thought it might be useful to have a signal when the frameworks release breaking changes.

KnorpelSenf commented 2 months ago

I guess I don't understand why the extra permissions are required then

KnorpelSenf commented 2 months ago

I didn’t pin the dependencies because I thought it might be useful to have a signal when the frameworks release breaking changes.

Agreed, nice

winstxnhdw commented 2 months ago

I guess I don't understand why the extra permissions are required then

Just to not fail the tests.

KnorpelSenf commented 2 months ago

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

winstxnhdw commented 2 months ago

@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 commented 2 months ago

the parameters

Which ones? I can't quite follow rn

winstxnhdw commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

Alright, agreed. Regarding https://github.com/grammyjs/grammY/pull/565/commits/aae3884ad95b4fa1938cb0cd31b4110959a8444c, 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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

Please consider signing your commits in the future :)

KnorpelSenf commented 2 months ago

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

allcontributors[bot] commented 2 months ago

@KnorpelSenf

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