Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ctx.send* methods and optional useNewReplies from telegraf/future #1623

Merged
merged 3 commits into from Aug 24, 2022

Conversation

MKRhere
Copy link
Member

@MKRhere MKRhere commented May 18, 2022

It has been a long-confusing feature of Telegraf that ctx.reply doesn't actually reply, but sends a message in the same chat. We now plan to fix it. This PR introduces ctx.sendMessage, ctx.sendPhoto, etc., that reflect the upstream API, but implicitly send a message in the same chat, more consistently with the other Context methods. This is the current behaviour of the ctx.reply methods.

This PR also introduces the new reply mode, which is optionally enabled using the useNewReplies middleware. Demonstration:

import { Telegraf } from "telegraf";
import { useNewReplies } from "telegraf/future";

const bot = new Telegraf(token);

// current behaviour:

bot.on("message", ctx => {
  // sends message in the same group, but doesn't reply
  ctx.reply("Hello");

  // sends message in the same group, but doesn't reply
  ctx.sendMessage("Hello");
});

bot.use(useNewReplies());

// new behaviour:

bot.on("message", ctx => {
  // actually replies to the message that was received
  ctx.reply("Hello");

  // sends message in the same group, but doesn't reply
  ctx.sendMessage("Hello");
});

This causes ctx.reply, ctx.replyWithPhoto, and the like to actually reply to the message in current context. If a message is not available, it will simply send a regular message.

A future minor release of telegraf will deprecate the current ctx.reply* methods in favour of the new ones, causing their usage to log warnings, prompting users to enable useNewReplies. v5 will completely replace the behaviour of ctx.reply with the new reply methods, and an optional middleware, useOldReplies, to replace them with the old methods may be added.

Copy link
Member

@wojpawlik wojpawlik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The history is a little messy already, I suggest merge-squashing or rebasing before merging.

an optional middleware, useOldReplies, to replace them with the old methods may be added.

Just throw away the old behavior, that's the point of a major release.

src/future.ts Outdated Show resolved Hide resolved
src/context.ts Outdated Show resolved Hide resolved
src/context.ts Outdated Show resolved Hide resolved
src/context.ts Outdated Show resolved Hide resolved
src/context.ts Outdated Show resolved Hide resolved
src/util.ts Outdated Show resolved Hide resolved
src/util.ts Show resolved Hide resolved
@KnorpelSenf
Copy link
Collaborator

I will review this in a few days, my time is currently a bit limited. Please don't wait for me if prefer to merge before I get to this.

@wojpawlik
Copy link
Member

I see no reason to rush this

@MKRhere MKRhere force-pushed the experimental-actual-replies branch from 2c67fb0 to 897275c Compare May 21, 2022 19:09
@MKRhere
Copy link
Member Author

MKRhere commented May 21, 2022

@wojpawlik

I suggest merge-squashing or rebasing before merging

I have rebased, but prefer not to squash history. Rather than treat git as a changelog, I prefer to treat it as a devlog.

Just throw away the old behavior

Okay, I've updated the PR description to reflect this

@KnorpelSenf

my time is currently a bit limited

No pressure; I might merge after some more testing to make sure nothing is breaking. If you get time meanwhile, appreciate a review.

@wojpawlik wojpawlik self-assigned this Jul 25, 2022
@wojpawlik wojpawlik force-pushed the experimental-actual-replies branch 2 times, most recently from 9c018b8 to da82c1f Compare August 6, 2022 17:42
@MKRhere MKRhere force-pushed the experimental-actual-replies branch from da82c1f to c9b8d92 Compare August 15, 2022 14:05
@MKRhere MKRhere changed the base branch from v4 to feat-api-6.2 August 15, 2022 14:06
@MKRhere MKRhere force-pushed the experimental-actual-replies branch 2 times, most recently from 2356817 to 68f5f03 Compare August 18, 2022 10:49
@MKRhere
Copy link
Member Author

MKRhere commented Aug 18, 2022

This PR is ready to merge into feat-api-6.2, and I'll bundle them into one release at 4.9.0. However, I'm not merging yet so as to not clog reviews on #1671. When #1671 is approved and ready to be merged, I'll merge this first.

@wojpawlik
Copy link
Member

Why not merge this into v4 right now?

Also: newReplies from telegraf/future to enable actual replies
Signed-off-by: Muthu Kumar <muthukumar@thefeathers.in>
Signed-off-by: Muthu Kumar <muthukumar@thefeathers.in>
@MKRhere MKRhere force-pushed the experimental-actual-replies branch from 68f5f03 to 78fc0bc Compare August 24, 2022 13:07
@MKRhere MKRhere merged commit 4b52522 into feat-api-6.2 Aug 24, 2022
@MKRhere MKRhere deleted the experimental-actual-replies branch August 24, 2022 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants