Shortcut for adding a middleware subtree

[IlyaSemenov]

Let's say I want to build a middleware tree:

∟ filter private chat
  ∟ filter text
  ∟ filter sticker
∟ filter group chat
  ∟ filter text
  ∟ filter sticker

If I understand correctly, that's how one achieves that:

const privateChatComposer = new Composer<BotContext>()
privateChatComposer.on("message:text", ...)
privateChatComposer.on("message:sticker", ...)

const groupChatComposer = new Composer<BotContext>()
groupChatComposer.on("message:text", ...)
groupChatComposer.on("message:sticker", ...)

bot.filter((ctx) => ctx.chat?.type === "private", privateChatComposer)
bot.filter((ctx) => ctx.chat?.type === "group", groupChatComposer)

I propose to create a helper such as Composer.create (or perhaps a global compose function) which allows to make the code above more concise:

bot.filter((ctx) => ctx.chat?.type === "private", Composer.create(bot => {
  bot.on("message:text", ...)
  bot.on("message:sticker", ...)
}))
bot.filter((ctx) => ctx.chat?.type === "group", Composer.create(bot => {
  bot.on("message:text", ...)
  bot.on("message:sticker", ...)
}))

implemented similarly to:

import { Composer, Context } from "grammy"

// Sample implementation as a global function.
// Could as well be a static method on Composer.
export function compose<C extends Context>(setup: (composer: Composer<C>) => void) {
  const composer = new Composer<C>()
  setup(composer)
  return composer.middleware()
}

---

UPDATE: based on the discussion below and prototyping, my current suggestion is:

bot.filter((ctx) => ctx.chat?.type === "private").setup(privateChat => {
  privateChat.on("message:text", ...)
  privateChat.on("message:sticker", ...)
})

Implemented like this:

export class Composer<C extends Context> {
  /** Run the provided setup function against the current composer. */
  setup(setup: (composer:this) => void) {
    setup(this)
    return this
  }
}

See monkey patch: https://github.com/IlyaSemenov/grammy-scenes/blob/87b5fd0b9153436bcf592bd47ff0e4e9bc8c208d/src/monkey_patches/composer_setup.ts

[KnorpelSenf]

No need. You can already do the following.

const privateChatComposer = bot.filter((ctx) => ctx.chat?.type === "private")
privateChatComposer.on("message:text", ...)
privateChatComposer.on("message:sticker", ...)

const groupChatComposer = bot.filter((ctx) => ctx.chat?.type === "group")
groupChatComposer.on("message:text", ...)
groupChatComposer.on("message:sticker", ...)

Does this work for you?

[IlyaSemenov]

Right, that is also possible, but it stills requires a separate variable per branch which is not super elegant. With 3 nesting levels, that will become barely readable. Arguably that is a non-issue though, as most of the time 3 nesting levels deserve to be decomposed anyway.

[IlyaSemenov]

What would you say about this?

bot.filter((ctx) => ctx.chat?.type === "private").compose(privateChat => {
  // compose() or setup() or something else
  privateChat.on("message:text", ...)
  privateChat.on("message:sticker", ...)
})

[KnorpelSenf]

Do you like this better than the current solution? If so, why? I'm not quite sure what the problem is that you're solving with this.

Either way, I wouldn't be declaring a second variable which is also called bot. I must admit I was pretty confused when reading your first snippet with Composer.create until I looked far to the right and saw that bot !== bot.

[IlyaSemenov]

If so, why?

Because it leads to simpler code and doesn't need a temporary module-level variable just for the sake of it.

I wouldn't be declaring a second variable

Fair enough. I updated the last example with proper naming.

[KnorpelSenf]

Do I understand you correctly that this effectively is the same this as lazy just without access to ctx? Means, I couldn't do anything conditionally at runtime?

Another difference to lazy is that this lazy let's you declare and return arbitrary middleware, while this gives you a composer instance to modify via side-effects. Correct? Does this mean this function is returning void?

What happens if a user passes an asynchronous function?

[IlyaSemenov]

Do I understand you correctly that this effectively is the same this as lazy just without access to ctx? Means, I couldn't do anything conditionally at runtime?

No, unlike lazy, this is not run-time but setup-time.

What happens if a user passes an asynchronous function?

What if user adds a middleware to the stack with an asynchronous function? That's not what users are supposed to do in the first place.

Effectively, it will update the future bot (outer middleware) behaviour, but that's kind of misuse.

[KnorpelSenf]

Do I understand you correctly that this effectively is the same this as lazy just without access to ctx? Means, I couldn't do anything conditionally at runtime?

No, unlike lazy, this is not run-time but setup-time.

Understood.

What happens if a user passes an asynchronous function?

What if user adds a middleware to the stack with an asynchronous function? That's not what users are supposed to do in the first place.

Effectively, it will update the future bot (outer middleware) behaviour, but that's kind of misuse.

We do support async ops during setup (using top-level await). There's also a protection in place (#14) which prevents misuse of this.

I understand you think the current solution is too cumbersome to work with. However, the suggested solution

• doesn't need significantly less code
• increases nesting (voluntary callback hell?)
• can easily be confused with existing lazy
• circumvents the surprisingly effective memory-leak prevention which helps newcomers to avoid a common mistake
• doesn't support top-level await

Generally speaking, allowing users to provide callbacks comes with a lot of internal challenges. This shows here.

[IlyaSemenov]

These are fair points, I'm not arguing. Can we still have it 'for advanced users' without advertising it extensively? After all it's simply about one more 3-lines method for a composer, which already has some other advanced methods. I'm sure most people don't use fork, for instance. (Not trying to say it's the same thing - I understand that fork adds unique functionality and the proposed compose/setup doesn't).

I personally use this often. I even added it to utils here: https://github.com/IlyaSemenov/grammy-scenes/blob/ec4612a8e4e6d5c2baf35dd913dda4458ffc1e1f/src/utils.ts but having a native composer instance method (rather than a global function) leads to even simpler code.

I'm okay if you're against it - I'll just come up with a monkey patching module, no big deal.

[KnorpelSenf]

Personally, I'm against adding this, but this is mostly because I don't find a nested setup function any more readable. However, this is of course subjective, and since grammY should do what the community thinks is best, I will consult other people about it to hear their opinion. I'll get back to you :)

[EdJoPaTo]

Personally I have to agree @KnorpelSenf here, multiple ways of doing the same thing should have good reasons or they will be a pain in the long run. Its not significantly simpler and, I understand this is something I personally do, when I have a bit of stuff for one composer, it will get its own file, where export const bot = new Composer<MyContext>() is basically the first line. Having many Composers with this approach is a nobrainer for me. (I probably loose some typing assumptions this way?)

With your simple example I can see that it might be slightly easier to write but it introduces interesting corner cases (which @KnorpelSenf already highlighted) which might end up causing additional pain. This might be extra fun when trying to understand projects of others because historically these corner cases are often seen as features. (There even is an xkcd for that). Also these benefits quickly vanish and are completely gone once multiple files are used (which happens rather quickly for me).

But it is awesome to think about different approaches so its great bringing up ideas how things could be improved. That will lead to discussions and growth so thanks for bringing up the idea @IlyaSemenov!

[KnorpelSenf]

A so far unnamed advantage of this suggestion is the local scoping. This doesn't necessarily affect readability (although it can), but it can help to avoid unintentional reuse of the composer instance at a later point in the source file.

Based on feedback by @Loskir

[wojpawlik]

https://ramdajs.com/docs/#applyTo (or a equivalent from a better library) already allows for

R.applyTo(bot.filter((ctx) => ctx.chat?.type === "private"), privateChat => {
  privateChat.on("message:text", ...)
  privateChat.on("message:sticker", ...)
})

[IlyaSemenov]

Let me add a bit of context to this. I am playing with my own implementation of scenes, and in the current API I ended up with the following. Consider a (reusable) captcha scene/conversation:

const captchaScene = new Scene<BotContext, { secret: string }>("captcha")

captchaScene.do(async (ctx) => {
  const { secret, image } = await generateCaptcha()
  ctx.scene.session = { secret }
  await ctx.reply(`Enter the letters you see below:`)
  await ctx.replyWithPhoto(image)
})

captchaScene.wait().on("message:text", async (ctx) => {
  if (ctx.message.text === ctx.scene.session.secret) {
    ctx.scene.resume()
  } else {
    await ctx.reply(`Try again!`)
  }
})

captchaScene.do((ctx) => ctx.reply("Captcha solved!"))

It looks (arguably) concise, but let's say user decides that they want to add a reaction to a sticker... So all of a sudden, they are forced to add a new module-level variable (or a weird functional wrapper):

captchaScene.do(async (ctx) => {
  const { secret, image } = await generateCaptcha()
  ctx.scene.session = { secret }
  await ctx.reply(`Enter the letters you see below:`)
  await ctx.replyWithPhoto(image)
})

const input = captchaScene.wait()
input.on("message:text", async (ctx) => {
  if (ctx.message.text === ctx.scene.session.secret) {
    ctx.scene.resume()
  } else {
    await ctx.reply(`Try again!`)
  }
})
input.on("message:sticker", (ctx) => ctx.reply("No stickers please."))

captchaScene.do((ctx) => ctx.reply("Captcha solved!"))

with the proposed improve, this could become:

captchaScene.do(async (ctx) => {
  const { secret, image } = await generateCaptcha()
  ctx.scene.session = { secret }
  await ctx.reply(`Enter the letters you see below:`)
  await ctx.replyWithPhoto(image)
})

captchaScene.wait().compose(input => 
  input.on("message:text", async (ctx) => {
    if (ctx.message.text === ctx.scene.session.secret) {
      ctx.scene.resume()
    } else {
      await ctx.reply(`Try again!`)
    }
  })
  input.on("message:sticker", (ctx) => ctx.reply("No stickers please."))
})

captchaScene.do((ctx) => ctx.reply("Captcha solved!"))

---

Now that I'm reading the actual code I suppose the local variable is not that big deal indeed. Even when user has more waits/inputs, I guess they can always name them accordingly (inputName, inputAge, etc.), and multiple scenes will live in multiple modules anyway.

The option of not having extra module-level variables still looks reasonable to me for certain cases, but I agree it's something I could live without.

[KnorpelSenf]

Thanks for the good ideas everyone. Even if this issue didn't lead to a PR, I'm looking forward to your next ones!