grammyjs / grammY

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

bug: aws-lambda webhookCallback adapter doesn't work #412

Closed chrisandrewcl closed 1 year ago

chrisandrewcl commented 1 year ago

I've started experimenting with grammyjs and encountered a few issues while trying to use the aws-lambda adapter for the webhookCallback:

  1. webhookCallback returns an async function, which causes the lambda to be called without the callback, resulting in "callback is not a function" errors.
  2. Webook replies don't work without explicitly setting Content-Type: application/json.
  3. Webhook replies work only for the first method call, which is confusing.

I was able to work around issues 1 and 2 by setting the header directly and wrapping the handler with a promise:

export const handler = async (event, ctx) => {
  let result = {};

  try {
    result = await new Promise((resolve) => {
      webhookCallback(bot, 'aws-lambda')(event, ctx, (error, response) => {
        if (error) {
          console.error(error);
        }
        resolve(response);
      });
    });
  } catch (error) {
    console.error(error);
  }

  /* Needed when replying directly to the webhook request */
  if (result?.body) {
    result.headers = {
      ...result.headers,
      ...{ 'Content-Type': 'application/json' },
    };
  }

  return result;
});

If my understanding is correct, the framework could be improved by:

  1. Supporting async/await.
  2. Setting the header when using webhook replies.
  3. I guess changing how the webhookCallback work would spill in a lot of other places and could be too much to ask, but it would be great to have more flexibility by having something as const reply = ctx.reply(/* ... */, { returnJsonPayload: true }) (disregarding canUseWebhookReply) so that we can explicitly set a response after we have done everything else. Is something like this possible in the current version? Without it, I am considering "leaking" the function given to canUseWebhookReply so that we can control when it should return true later on each request, but it seems messy. I hope there is a better way.

1 and 2 look like good "first issues", so I would like to contribute with a PR if that's ok.

KnorpelSenf commented 1 year ago

Absolutely! I'm looking forward to your contributions :)

I believe @amberlionk to be the one who contributed the AWS adapter in #62. In case you have any questions about the implementation, I'm sure we are both open to answer them!

KnorpelSenf commented 1 year ago

Sorry for the late reply btw, I was a little busy recently

KnorpelSenf commented 1 year ago

@all-contributors add @chrisandrewcl for bug

allcontributors[bot] commented 1 year ago

@KnorpelSenf

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

chrisandrewcl commented 1 year ago

@KnorpelSenf I've submitted a PR for review, addressing points 1 and 2.

Regarding point 3, is there any way to call an API method on "dry-run" to get only the JSON payload?

If not, and if it is something that makes sense for the framework, I can submit a separate feature request (to be considered for a future version) and this issue can be hopefully closed with PR#418.

KnorpelSenf commented 1 year ago

Regarding point 3, is there any way to call an API method on "dry-run" to get only the JSON payload?

I'm not 100 % sure I understand what you mean, but I believe https://grammy.dev/advanced/transformers.html to be the solution.