feat: deprecation message when an async callback is used in a Suite #4921
Conversation
|
|
test/integration/fixtures/suite/suite-async-callback.fixture.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Franco Victorio <victorio.franco@gmail.com>
|
@alcuadrado thank you, I will review within the next few days. I certainly don't agree with the type of warning (deprecation), since we have never offered a feature "asynchronous describe". |
|
@wdavidw are you still working with Mocha and CoffeeScript?
Does this PR work with CoffeeScript? What happens if the last |
I copied this from the previous implementation. I guess the reasoning behind that is that some users are getting away with async callbacks now, and a fatal error can be seen as breaking by them. |
juergba
added
area: usability
There was a problem hiding this comment.
@alcuadrado I'm quite sure this change will trigger deprecation warnings to CoffeeScript users in certain circumstances. Anyway this PR is much softer than the original proposal and there is a work-around by adding a return statement to the suites. Otherwise they will have to live with a single deprecation warning.
Could you please do some manual browser testing, since we don't run our integration tests on browsers. And confirm the outcome, thank you.
| const suiteResult = opts.fn.call(suite); | ||
| if (suiteResult instanceof Promise) { | ||
| errors.deprecate( | ||
| 'Suite "' + |
There was a problem hiding this comment.
Please add "[mocha] Suite ..." to the message as done in other parts of our code. The origin of a warning is clearer that way.
There was a problem hiding this comment.
@alcuadrado ping - can you make this change?
There was a problem hiding this comment.
Looks good as a general re-implementation of #3550! Heck, I even think we should go further and fully re-implement to check all truthy values - not just Promises.
Thoughts?
| @@ -145,7 +145,16 @@ module.exports = function (suites, context, mocha) { | |||
| throw createUnsupportedError('Pending test forbidden'); | |||
| } | |||
| if (typeof opts.fn === 'function') { | |||
| opts.fn.call(suite); | |||
| const suiteResult = opts.fn.call(suite); | |||
| if (suiteResult instanceof Promise) { | |||
There was a problem hiding this comment.
Going back to https://github.com/mochajs/mocha/pull/3550/files#diff-28d67f27fa361db19b8a3aa9181d89c5f0d5ae1089167c55f0341675386e3a28R115, I think we no longer need to prioritize supporting CoffeeScript users. The implicit returns that caused #3550 to be reverted in #3759 are not a good idea for the exact reason we saw here: that they unintentionally return values even when those values shouldn't be returned!
| if (suiteResult instanceof Promise) { | |
| if (suiteResult !== undefined) { |
https://typescript-eslint.io/rules/no-confusing-void-expression is the right direction to go IMO.
JoshuaKGoldberg
added
the
status: waiting for author
JoshuaKGoldberg
changed the title
I'm sending this PR after having proposed this feature in #4920, getting some positive reactions, and researching the history of a similar feature that was removed
Description of the Change
This PR introduces a change that prints a depreciation message if an
asynccallback is used in aSuite.Alternate Designs
v6.0.0introduced a similar feature, but it printed a deprecation warning if aSuitereturned any value other thanundefined, which was not practical in some cases (see #3744), so it was removed.This PR is a specialization of that feature that prints a warning when
Promises are returned instead.Also, I originally proposed throwing a fatal error, but that would be a breaking change, so I changed it to a deprecation message. This same criteria was applied to the previous version of this feature.
Why should this be in core?
I don't think this can be implemented outside of core.
As this PR benefits users who don't have a clear understanding of how a
Suiteworks, taking this functionality out of core will have much lower, as there wouldn't be a clear path for those users to discover how to enable it.Benefits
Many users don't realize that Suite's callbacks can't be
async. In some cases, they get away withasynccallbacks, which further increases the confusion.This PR is intended to clarify that confusion.
Possible Drawbacks
I don't think this version of the feature has any serious drawback.
It may print deprecation warnings for people who were using async callbacks in
Suites with some success (and a ton of luck), but those users were already at high risk of their code breaking. This change is intended to help them.Applicable issues
Fixes #4920
semver-patch
(copying the semver criteria of #3550)