Should we contain to create callbacks for new APIs?

[TheDudeFromCI]

This topic was started in this PR: #1528 (review)

Currently, Mineflayer is moving away from callbacks and moving towards using Promise-based code. The main codebase has already been converted over, and most other modules and plugins for Mineflayer are planned to follow very soon.

The question is should new APIs also create new callbacks for them, or should we encourage users to move away from callbacks completely? Existing callbacks will remain either way simply for backward compatibility.

Our options are to continue to create callbacks for all new APIs, or depreciate the use of callbacks entire and only maintain existing ones for backward compatibility only.

[stenlan]

In my opinion, callbacks are there just for backwards compatibility. Any new features or breaking changes to existing features should incorporate only promise versions and not callback versions, as this will only encourage sticking to the callback system, which IMO should be slowly deprecated and then phased out.

[TheDudeFromCI]

Personally, I agree with this. Moving to use solely promise-based designs will produce more intuitive and readable code, especially with how asynchronous the Mineflayer API is.

await bot.dig(block)
bot.setControlState('jump', true)
await bot.waitForTicks(10)
bot.setControlState('jump', false)
await bot.placeBlock(bot.entity.position.offset(0, -1, 0))

This is significantly more readable and beginner-friendly than callbacks would be. I notice that a lot of new users don't understand the async nature of Mineflayer, so they'll tend to write things like:

bot.dig(...)
bot.dig(...)
bot.dig(...)

then get confused why it's not working. With promises, just write the word await in front, and async in the function initializer, and it practically works exactly like you'd expect it to.

Personally, I would go as far as printing a depreciation warning on startup when using callbacks.

[wvffle]

Agreed.

[wvffle]

Yes, we have the callbacks API for backwards compatibility though, if a developer who feels better with callbacks comes to us and see that we support both approaches - how much disappointment would he have when he finds out that the code he's been trying to write for last 2 hours with callbacks is only promise API?

In my opinion, if we're supporting both approaches, then we are supporting them both.

Like @TheDudeFromCI said, we can print a deprecation warning on first occurance of callback function. Also maybe on install? Provided we ARE going to deprecate the callback API.

[stenlan]

Indeed, so I would lean towards deprecation of callbacks to discourage users in continuing to use them.

[TheDudeFromCI]

I believe this also covers examples, as well. If we decide to support both, we'll have to maintain examples that cover both use cases.
(i.e, callback-based versions and promise-based versions of the same bots.)

[rom1504]

I think we should indeed deprecate the callback API.
However I think we should only do it once we are confident there are no remaining issue for the promise API.
To validate this I think it's useful to:

• use it ourselves: our tests and our plugins should use it Promisify follow up #1482
• make sure at least some users are using the promise API without issue

Once this is true, we can consider adding deprecation warnings and tell users not to use callback API (if someone wants to prepare a PR to do this that we won't merge right now, feel free)

For the specific question of small new features, I'd say let's not add the callback version. Users wanting to use it in a callback-based app, can always do function().then(() => cb())

[wvffle]

What you're saying is a contradiction in my opinion.

However I think we should only do it once we are confident there are no remaining issue for the promise API.

For the specific question of small new features, I'd say let's not add the callback version.

Isn't adding new promise specific features a first step to deprecate callback API?

[rom1504]

yes it is indeed a beginning of deprecation. For new features we already know they work well in their promise form, as they are added in that form first. We also know they have no callback users. So it seems safe to deprecate callbacks for new features.
I just want to hold back a little bit for the old features as they have many callback users and not yet a lot of promise users, so we should make sure we did not miss any bug there.

[TheDudeFromCI]

Okay, this seems like a good direction for us to move in.
Long-term plans to phase out callbacks, but keep them maintained for a while until we are confident in how well they work and how much of our userbase is now using them.

I agree.