Update no-callback-in-promise.md

[marcogrcr]

Add documentation of why callbacks shouldn't be invoked inside Promise's then() and catch() methods.

What is the purpose of this pull request?

• Documentation update
• Bug fix
• New rule
• Changes an existing rule
• Add autofixing to a rule
• Other, please explain:

What changes did you make? (Give an overview)

Added documentation to provide more context to people that want to understand the purpose of this rule.

[xjamundx]

So this is a great explanation of a bug that can occur when mixing these two types of patterns. My purpose for introducing this rule was more to encourage teams to pick a common pattern such as callbacks, async/await or promise and stick with it for consistencies sake, but this is a decent explanation and honestly pretty well thought out.

[xjamundx]

while this works, i don't love these either, and would prefer that folks promisify'd their code instead of doing this

[xjamundx]

I'm pretty ok with this merging as-is, but would appreciate any thoughts on if we should even encourage the setImmediate approach

[marcogrcr]

Hello @xjamundx. Thanks for the review and the comments!

I was really surprised when I triggered this rule for the first time and I had to search online what was wrong with mixing callbacks and promises.

I read the source code of nodeify to see how did they "solved" the problem, and to my surprise they wrapped the callback invocation in a deferred execution function (e.g. setImmediate). I was still not sure what was this fixing until I came across this article that did a pretty good job at explaining what was wrong with it.

Both this original documentation and the article refer to using a library to fix the problem. However for curious people like me that wanted to understand what was wrong in the first place and how the library fixed it this was not enough. I wanted a more detailed explanation which motivated me to write this PR.

While I agree that folks should avoid mixing both programming styles sometimes this is unavoidable. Take for example the AWS.Credentials.prototype.refresh() method. Creating a subclass of AWS.Credentials requires this method to be overridden which uses a callback style approach. Everywhere else in my code I was using Promises (e.g. async function) so the third-party library forced me to mix them if I wanted to stick with them.

Is there a simpler way of implementing this method if I want to stick with Promise that does not require to wrap the call in setImmediate or similar?

I'm happy to update the guidance with something simpler if available.

[marcogrcr]

Hey @xjamundx any update on this?

[xjamundx]

@marcogrcr let me run some tests tonight to make sure your recommendations don't cause any lint warnings

[xjamundx]

With the current implementation these all fail:

    'whatever.then((err) => { process.nextTick(() => cb()) })',
    'whatever.then((err) => { setImmediate(() => cb())) })',
    'whatever.then((err) => setImmediate(() => cb()))',
    'whatever.then((err) => process.nextTick(() => cb()))',

But I'll just mention that in the docs and we can file a separate bug to support those cases.

[xjamundx]

Published with a few changes in + eslint-plugin-promise@5.1.1