grammyjs / grammY

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

Multi Sessions and Conversations #345

Closed receter closed 1 year ago

receter commented 1 year ago

I don’t manage to get conversations to work with multi sessions. It does work with normal configuration (not session type "multi") but if I try to change it to multi it stops working.

  bot.use(session({
    type: "multi",
    user: {
      storage: new MemorySessionStorage(),
      initial: createInitialSessionDataUser,
      getSessionKey: (ctx) => ctx.from?.id.toString()
    }
  }));

  bot.use(conversations());
  bot.use(createConversation(conversationA));

This works though:

  bot.use(session({
    initial() {
      // return empty object for now
      return {};
    },
  }));

  bot.use(conversations());
  bot.use(createConversation(conversationA));
KnorpelSenf commented 1 year ago

Remember that when you say it “doesn't work”, we cannot login to your computer and figure out what's wrong. You need to explain what you are expecting to happen and what is actually happening :)

KnorpelSenf commented 1 year ago

Are there any errors? What is the behaviour? Does it compile?

receter commented 1 year ago

Thanks for your fast reply. I really like the library and the docs, I had a lot of fun getting into it already!

If I use lazySessions it does throw an error. With just session it does not throw but the conversations do not seem to work either.

Here is a demo repository with a minimal example of the issue: https://github.com/receter/grammy-multi-sessions-conversations

KnorpelSenf commented 1 year ago

Multi sessions require you to specify all fragments of the session upfront. This means you'll also have to specify conversation here: https://github.com/receter/grammy-multi-sessions-conversations/blob/7a31d2d19babfa990d41f668e4304711934fe497/src/configureTelegramBot.js#L18-L22

This should probably go in the conversation docs.

receter commented 1 year ago

This should work right?

  bot.use(lazySession({
    type: "multi",
    test: {},
    conversation: {}
  }));

It still throws:

  error: TypeError: Cannot read properties of undefined (reading 'conversation')
      at Object.session (/Users/andreasriedmuller/PhpstormProjects/grammy-multi-sessions-conversations/node_modules/@grammyjs/conversations/out/conversation.js:259:36)
receter commented 1 year ago

But this seems to be an issue with lazySession only, it does work with session.

  // works
  bot.use(session({
    type: "multi",
    test: {},
    conversation: {}
  }));
KnorpelSenf commented 1 year ago

Can you share the repro? In other words, can you share a minimal example (~20 lines posted in this thread) illustrating the problem? I don't feel like digging through another complete repository again :)

KnorpelSenf commented 1 year ago

Hang on, is this only breaking when you're using lazy multi sessions? If so, then the problem is that there are no lazy multi sessions. This isn't supported. We implemented it initially, but combining lazy sessions with multi sessions leads to numerous strange edge cases that are hard to understand and by design impossible to mitigate, so we dropped this again. If you were using TypeScript, your editor could've told you this. Since you don't seem to be using it, here is a link to the API reference of grammY which tells you this: https://deno.land/x/grammy@v1.12.4/mod.ts?s=lazySession

I'll add another example for this to the docs in the coming days. There is no bug here.

I will close this issue. Feel free to comment or reopen if you have further questions :)

receter commented 1 year ago

Ok, that explains it, thank you! I might switch to Typescript...

KnorpelSenf commented 1 year ago

Can recommend! I'm sure you've already seen the getting started guide which walks you through a working TypeScript setup in just a few minutes.

By the way, there's also something called Deno which makes TypeScript development substantially easier because you need 0 config. There's a short write-up about that as well: https://grammy.dev/guide/introduction.html#prerequisites-to-getting-started

Hope it helps, good luck with your project!

KnorpelSenf commented 1 year ago

Check out https://github.com/grammyjs/website/pull/563 for the docs update and #347 for a change to the library that detects and prevents this mistake from happening again for non-TS people.

receter commented 1 year ago

Maybe add a warning to the sessions docs as well.

I will just skip using multi sessions, a session per user should be all I need for now.

We implemented it initially, but combining lazy sessions with multi sessions leads to numerous strange edge cases that are hard to understand and by design impossible to mitigate, so we dropped this again.

Is there a discussion about this somewhere? made me curious.

KnorpelSenf commented 1 year ago

Of course, all development happens publicly and it's supposed to be transparent. This includes all discussions and brainstorming sessions.

The conversation you're asking for starts here: https://github.com/grammyjs/grammY/pull/216#issuecomment-1201078050