Better separation of init and start

[IlyaSemenov]

Currently, init() and start() are too heavily coupled which makes it harder to extend them.

My use case is:

1) I want to init() and start() bot separately. That is, because by calling init() I make sure the token is correct etc. and must do some housekeeping before entering the (endless) start(). Also, if I understood correctly if I switch to webhooks I will not be calling start() at all yet I will need init()? 2) I want to subclass Bot with and override init() with some extra code.

Currently, init() is called unconditionally in start: https://github.com/grammyjs/grammY/blob/dc0df4702bf49261239e14ba4d9ce18f3117fc0a/src/bot.ts#L305-L307

init() itself prevents double call by checking this.me: https://github.com/grammyjs/grammY/blob/dc0df4702bf49261239e14ba4d9ce18f3117fc0a/src/bot.ts#L215-L218 (but it still warns that the bot has been initialised!)

However, it is not possible to use that in a subclass, because this.me is private: https://github.com/grammyjs/grammY/blob/dc0df4702bf49261239e14ba4d9ce18f3117fc0a/src/bot.ts#L131

What I suggest is, at the very least, prevent start() from calling init() if it has been called already.

[KnorpelSenf]

I'm not quite sure I can follow yet. Could you provide some code that illustrates why/how you're extending the class?

What I suggest is, at the very least, prevent start() from calling init() if it has been called already.

Duplicating the check form init to start is not very DRY, that's why I'm against it until I understand why it's necessary. Also, as of now, init is idempotent. Couldn't we just return a boolean indicating whether a fetch call was performed? That way, you can call super.init and find out whether or not any actions need to be performed. I would also be okay with a protected me.

[EdJoPaTo]

Personally I worked around this by just setting up the bot (const bot = new Bot), then checking bot.api.getMe(). The exact call is irrelevant to me there, getMe is simple and verifies the token is working. Then I do my own init logic which takes a while and when that is done bot.start is executed. This doubles the call of bot.api.getMe() but its a relatively small cost to pay.

I mainly added the bot.api.getMe() to have a "fast fail" when the bot is broken so I dont need to wait ages until my own init logic is finished.

[KnorpelSenf]

Can't you run bot.init() instead of performing the API call? It will throw the same error, but save you the second API call.

[IlyaSemenov]

Could you provide some code that illustrates why/how you're extending the class?

Simplified example:

export class FooBot extends Bot<FooBotContext> {
    constructor(public readonly foo: FooModel) {
        super(foo.token)
        // I actually do this.use(...) here but it's irrelevant to the example
    }

    async init() {
        await super.init()
        // This will be called twice
        await this.api.setMyCommands([{ command: "start", description: "Start" }])
        await this.foo.$patch({ ok: true })
    }
}

async function run_bot(foo: FooModel) {
    const bot = new FooBot(foo)
    await bot.init() // expected to throw if something is not well
    subscribeToExternalEvents(() => { /* ... */ })
    await bot.start()
}

In the actual code, it's a set of bots that init (after which the set is reduced to only those that were able to init), then some processing, then start.

Please don't get me wrong: I know how to work around this. This is not a support request, I'm just showing the lack of flexibility in this regard. If you don't consider this a shortcoming, that's fine, the issue can be closed.

Also, as of now, init is idempotent.

It's actually not fully idempotent. It has a side effect (a debug warning).

Currently init and start are mutually exclusive. Even though the user can call them successively, it's not how they are supposed to perform. Which is odd, because testing/initialising the bot (and possibly doing something else, such as simply logging that the bot is fine) before the endless loop starts is arguably a popular scenario and deserves a first class support.

Couldn't we just return a boolean indicating whether a fetch call was performed?

We could, but IMO that's more hackish.

This doubles the call of bot.api.getMe() but its a relatively small cost to pay.

Thank you.

Again, I'm capable of inventing all sort of hacks - but since this is a library in active development, I am trying to show when those hacks are needed which sometimes is a sign that API could be improved.

[EdJoPaTo]

While thinking about improving something: I also have bots which only send and do not listen for updates. I use bot.api.getMe() to ensure the bot is working when needed but never call start because I do not need the updates. Maybe calling bot.init() there would also be a good idea. Not sure if it is possible to omit the init completely by integrating it into the constructor (or providing an async factory).

[KnorpelSenf]

Thanks for the code example, now we're on the same page.

Again, I'm capable of inventing all sort of hacks - but since this is a library in active development, I am trying to show when those hacks are needed which sometimes is a sign that API could be improved.

I'm grateful you do, and I see that this could be improved. There are plenty of small solutions to this, and while I do not like a few of them (e.g. a public me), I do not have strong opinions about any of the other. What do you think about #155?

[IlyaSemenov]

155 seems quite reasonable to me, and will allow my use case. Thanks!

[KnorpelSenf]

Awesome! I'll push out a patch release after fixing the nitpick.

[KnorpelSenf]

@all-contributors add @IlyaSemenov review

[allcontributors[bot]]

@KnorpelSenf

I've put up a pull request to add @IlyaSemenov! :tada:

[KnorpelSenf]

The fix for this (#155) was released in 1.6.2.