v5 menu select fails to update when menu is deleted

[lamuertepeluda]

Describe the bug I want just to report that probably this method needs some little exception handling, maybe just wrap it with a try/catch

menu.select(...
   set: async (ctx, selectedKey) => {
       // do stuff, like update cache, remote server values...
       // Now I need to close the menu. I am using this method (although I tried with ctx.deleteMessage() and it's just the same)
       await deleteMenuFromContext<Context>(ctx);
   })

This leads to unhandled promise rejection

(node:21113) UnhandledPromiseRejectionWarning: Error: 400: Bad Request: message to edit not found
    at /home/lamuertepeluda/Development/my-awsome-bot/node_modules/telegraf/core/network/client.js:281:17
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at async Promise.all (index 0)
    at async MenuMiddleware.editMenuOnContext [as _sendMenu] (/home/lamuertepeluda/Development/my-awsome-bot/node_modules/telegraf-inline-menu/dist/source/send-menu.js:55:13)
    at async /home/lamuertepeluda/Development/my-awsome-bot/node_modules/telegraf-inline-menu/dist/source/menu-middleware.js:69:17
(node:21113) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:21113) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Versions Did you updated telegraf and telegraf-inline-menu to the latest version? "telegraf": "^3.38.0", "telegraf-inline-menu": "^5.0.0-beta.5", "telegraf-session-redis": "^5.1.0", "telegraf-stateless-question": "^0.1.1", "telegram-format": "^2.0.0"

What version of NodeJS (and TypeScript) do you use?

• Version of NodeJS: 12.18.0
• Version of TypeScript: 3.9.3

[EdJoPaTo]

Looking onto this is interesting as to what is perceived here about the library currently.

First the choice of using select here: The idea of select is to be a specialized choose: provide the user with a selection of multiple choices and give a visible feedback when the user selects one of them: isSet is queried and the menu is updated to show the user what he did: select one. Deleting the menu within the select set is trying to do the opposite of that: don't show a visual feedback of what the user did. This throws an error as the middleware tries to update the menu in order to show what the user did. But as the message is gone this fails. Maybe there needs to be a better explanation on what the idea of select is?

The error handling: In my opinion a library should catch errors which can be handled and are not relevant to the user. For example when the menu is updated but the message content doesnt change the Telegram API returns an error. This error is catched by the library so the user does not need to handle it as everything is fine for the user: the menu has the state the user wants. In this case the error is not recoverable so it just throws it. The user has to handle it. In this case you have to await and try/catch editMenuOnContext yourself. When using telegraf and middlewares / your functions have errors you still need to catch them. This can be done via bot.catch.

As I said its interesting to see what others perceive differently to the thoughts I had behind things. How can the library be improved to be easier understandable? Any suggestions? :)

[lamuertepeluda]

Hi @EdJoPaTo,

thanks for sharing your thoughts about the design of the select. My use case it's quite simple: I have a 1-depth menu, consisting of a select menu with several options, including a "close" button corresponding to "none of those". I was giving a feedback to the user via ctx.reply in the set method, right after removing the menu with the deleteMenuFromContex. E.g. "You have selected: option". If the user presses the Close button, there is no need of showing anything. I don't need to leave the menu open after the user performs a choice, since afterwards the interaction can happen textually, or via a command. And then some commands or text may trigger this menu or a different one, if needed. I am calling the middleware directly, i.e. with the replyToContext method in response to some user text or action.

I did try wrapping the call to deleteMenuFromContext(ctx) in a try/catch block, but the exception is still thrown. Perhaps I should use this other editMenuOnContext method as you wrote.

But I think offering a way to close a menu, or documenting more clearly the way to do it, can be helpful (btw. I've seen now a new item about it in the FAQs, thanks).

Or did I get the select() purpose wrong and perhaps I should have just used choose() instead in this case?

[EdJoPaTo]

I thought I already responded to this one.. sorry about that.

Not the deletion of the message throws the error. The error is throws when the menu tries to do the job: showing the selection to the user by updating the menu. Updating a message which isnt there anymore. The error happens within the MenuMiddleware, more precisely within the editMenuOnContext function called by the MenuMiddleware. Its always a good idea to catch possible errors with bot.catch.

Taking a look about your choices towards the user close seems like a 'non' choice, something different. From only knowing that I would tend towards using a different button for that besides the selection / user choice.

select is basically a specific way of a choose: let the user choose something but also show the user afterwards what was done. If you dont want to use the feature of the select and do it yourself / differently then just stick to the more generic method choose there.

[lamuertepeluda]

I see what you mean now. I kind of need something in between, that's why I got confused.

I needed the select behavior of having the current choice checked (if any). But I also needed to show a message once the user pressed a button, removing the menu and showing a string.

My desired behavior should also be easy to accomplish with choose by manually adding a checkmark on the labels using array.map, I guess.

Hower having a way to display a "current choice" label automatically wouldn't be bad

[lamuertepeluda]

I can confirm that using choose resolved the error I was getting, and the behavior I am getting is the desired one.

In order to display the checkmark, I have used the prefixEmoji function.

import { prefixEmoji } from 'telegraf-inline-menu/dist/source/prefix';

Would it be possible to export prefixEmoji in the main package export?

[EdJoPaTo]

I thought about explicitly returning the target menu afterwards in set functions too. If it is required to return something you probably want to update the current menu most of the times anyway but can do differently. #100 seems very helpful in this case too.

For example if you want don’t want to update the menu as you deleted it or to go to a parent menu as you offered the selection in a child menu.

Regarding the export of prefix: not sure about it. I think the main export should contain the main components. More special things should end up in different imports. Maybe the prefix logic should even be a different npm package.

[lamuertepeluda]

That makes sense. However in my case I did not need to return, since the menu is a simple choice of options (or Cancel) and the the bot register the choice, removes the menu and writes a text feedback (e.g. "You choose X" or "Canceled").

So in my case having a void function is fine, and also intentional.

I agree, having the prefix logic together with other utility functions in a separate package is also right. Or at least in a separate export, e.g.

import { prefixEmoji } from 'telegraf-inline-menu/utils';

instead of

import { prefixEmoji } from 'telegraf-inline-menu/dist/source/prefix';

[EdJoPaTo]

I would like the import to be import { prefixEmoji } from 'telegraf-inline-menu/prefix'; but that would require different packaging and is currently more effort than it would solve in my opinion. As long as thats the actual path within the package we are stuck with the current paths there.

I your case you want to not update the menu afterwards and tell the menu 'I'm fine with not updating'. Currently the menu is just always updated. Having the customisation there is what the idea of #100 is about.

[EdJoPaTo]

with rc-1 you can use menu.select and just return false so the menu is not attempted to update afterwards. This way you have all the other features of select with your usecase of deleting the menu.

@lamuertepeluda can you check it out?

[lamuertepeluda]

Hi @EdJoPaTo , it works fine now: no exception thrown. Thank you.