grammyjs / conversations

Conversational interfaces for grammY.
https://grammy.dev/plugins/conversations
MIT License
52 stars 17 forks source link

Error while exiting conversation from `waitForCallbackQuery` #106

Closed Peterculazh closed 6 months ago

Peterculazh commented 6 months ago

Hello, I'm getting crash during exiting conversation

TypeError: Cannot read properties of undefined (reading 'change_language')
    at ...\node_modules\@grammyjs\conversations\out\conversation.js:373:45
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at ...\node_modules\grammy\out\composer.js:61:13
    at ...\node_modules\@grammyjs\conversations\out\conversation.js:381:17
    at ...\node_modules\grammy\out\composer.js:61:13
    at ...\node_modules\@grammyjs\conversations\out\conversation.js:272:9
    at ...\node_modules\grammy\out\composer.js:61:13
    at ...\node_modules\grammy\out\convenience\session.js:65:9
    at ...\node_modules\grammy\out\composer.js:61:13
    at ...\node_modules\grammy\out\composer.js:56:9

Reproduction: firstly I tried to use drop parameter to exit conversation, but seems it's not related to actually exiting and I decided to use ctx.conversation.exit to actually exit conversation. There a example of otherwise callback:

await conversation.waitForCallbackQuery([Language.en, Language.ua], {
    otherwise: async (ctx) => {
        await ctx.conversation.exit('change_language');
        await ctx.reply(localization.t('changeLanguage.pressedWrongDuringChanging'));
    },
});

I've looked up in source code and that occurs because of session.conversation[id].length === 0 where session.conversation is undefined

Fix looks just like that session?.conversation && session.conversation[id].length === 0, but I'm not figure out how I would can run tests for checking up is change broke something or not?

KnorpelSenf commented 6 months ago

ctx.conversation generally isn't available inside conversations, even though it may sometimes accidentally. Sadly, there is no way to fix this such that it can work reliably in all cases. The solution to this problem is to work on #53 instead.

Peterculazh commented 6 months ago

ctx.conversation generally isn't available inside conversations, even though it may sometimes accidentally. Sadly, there is no way to fix this such that it can work reliably in all cases. The solution to this problem is to work on #53 instead.

So currently only way to exit conversation is throw error and catch? Is conversation deletes properly from array of conversations in case of throwing error inside otherwise?

Works fine in such way, but now I'm worrying about not storing a N-amount of conversations because of throwing error :sweat_smile:

try {
    await conversation.waitForCallbackQuery([Language.en, Language.ua], {
        otherwise: async (ctx) => {
            // await ctx.conversation.exit('change_language');
            await ctx.reply(localization.t('changeLanguage.pressedWrongDuringChanging'));
            throw new Error(`trying to exit conversation`);
        },
    });
} catch (error) {
    // catches error and returns
    console.log(error);
    return;
}

PS. I figured out that with return at catch it actually leaving conversation?

KnorpelSenf commented 6 months ago

I'd just handle all updates and avoid running into otherwise when I want to leave a conversation, but yes, from within otherwise, there currently isn't another way. The APIs are definitely still lacking there.

now I'm worrying about not storing a N-amount of conversations because of throwing error

I don't understand this part

with return at catch it actually leaving conversation?

Yes

Peterculazh commented 6 months ago

now I'm worrying about not storing a N-amount of conversations because of throwing error

I don't understand this part

Meant that in case of not properly leaving conversations and not clean them, they stays in memory forever and eventually will occur memory out.

I'd just handle all updates and avoid running into otherwise when I want to leave a conversation, but yes, from within otherwise, there currently isn't another way

Got it, thanks, will think about not using otherwise.

Feel free close that issue

KnorpelSenf commented 6 months ago

now I'm worrying about not storing a N-amount of conversations because of throwing error

I don't understand this part

Meant that in case of not properly leaving conversations and not clean them, they stays in memory forever and eventually will occur memory out.

Conversations are stored in a database, they only consume ram while they're being executed or if you store your sessions in memory

I'd just handle all updates and avoid running into otherwise when I want to leave a conversation, but yes, from within otherwise, there currently isn't another way

Got it, thanks, will think about not using otherwise.

Feel free close that issue

Alright