grammyjs / grammY

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

feat: integrate the auto-thread plugin into the core lib #352

Open EdJoPaTo opened 1 year ago

EdJoPaTo commented 1 year ago

ctx.reply() (and similar reply methods) do not reply to the current forum thread. They should as their use is to be a reply to whatever the current context is.

Not sure if this is a breaking change or not as it personally sounds like a bugfix to me.

Workaround until fixed:

await ctx.reply("something", {
  message_thread_id: 314,
});
KnorpelSenf commented 1 year ago

This can already be done using https://github.com/grammyjs/auto-thread

EdJoPaTo commented 1 year ago

Personally I still think this is something the reply method is supposed to do on its own. The idea of reply is to reply to whatever the update was about. If that's in some thread then reply in some thread. If the reply method is not handling this its a bug to me as the idea of reply is violated.

Integrating some plugin sounds strange to me as its manipulating via a middleware and not natively correct behaviour which would be more efficient. Maybe that is what was meant to be done but the title of this issue should still be something like "reply doesnt work correctly with threads → fix that"

roziscoding commented 1 year ago

I agree that this should be the default behavior, but changing this now would lead to a change in behavior of existing bots and would require people who like the current behavior to refractor their code to use ctx.api.sendMessage, or ctx.sendMessage, if we add it.

Also, it'd require us to change the bahavior of all the replyWith... too, for consistency, which would require us to have ctx.send... methods for people who like the current behavior.

If we were to integrate this behavior into core, I'd do it being a config flag, and then we make it default to true on 2.0.

KnorpelSenf commented 1 year ago

The current behaviour can be achieved via ctx.api methods. This is preferable over configuration.

Is it correct to leave out the parameter? It used not to be, but perhaps leaving it out will now send messages to the general topic?

I see two possible ways to proceed with this:

  1. Wait until 2.0.
  2. Label this a bug because it's unexpected behaviour, so we're allowed to fix it immediately without breaking semver.
roziscoding commented 1 year ago

The current behaviour can be achieved via ctx.api methods. This is preferable over configuration.

Yes, which means that, if we do point 2, we'll change the behavior of people's code without changing the major, and will require people to refactor their codebase to stay up to date, which should only really be expected on major releases

I understand that asking people to refactor is preferred over configuration, but that would break SemVer. If we're OK with breaking SemVer because we assume everybody expects reply to work this way, then fine. Otherwise, I'd suggest we go another route.

I'd also like to point out that using ctx.api methods will require the user to pass the chatId manually, and there won't be a method to send a message to the current chat without replying. If we do 2, we need to keep that in mind, and probably on the docs.

KnorpelSenf commented 1 year ago

Yes, which means that, if we do point 2, we'll change the behavior of people's code without changing the major

I'm aware, the question remains if that isn't actually a good thing. If it's true that omitting the thread identifier refers to the general topic, then the current implementation is somewhat broken. If a user sends a message in a topic, the bot would always send an unrelated message in a different topic. Fixing ctx.reply would fix these bots.

EdJoPaTo commented 1 year ago

Personally I still think this is a bug. Reply is meant to reply to exactly what the user did, so sending a message in a thread should be replied exactly there: in the thread. The current behavior does not respect that and differs from that → bug.

I also cant see why people would want the current behaviour. reply is simply useless for threads currently and the dev needs to specify the thread id so the reply method works as expected. Fixing that in grammY will not require refactoring of bots as both the dev and grammy set the same thread id. The dev can remove that additional code but it will not break something. Or do I overlook something?

roziscoding commented 1 year ago

I think I mixed things up. @KnorpelSenf linked to this when I talked about joining the autoquote and autothread plugins, so I assumed we would integrate the autoquote plugin into core too. I'm sorry. I completely agree that the autothread plugin should be incorporated into core.