ESLint Config Migration: Fixing consistent-return linter warnings #1075
Conversation
| @@ -695,6 +695,8 @@ export default class App { | |||
| } catch (error) { | |||
| this.logger.warn('Authorization of incoming event did not succeed. No listeners will be called.'); | |||
| error.code = 'slack_bolt_authorization_error'; | |||
| // disabling due to https://github.com/typescript-eslint/typescript-eslint/issues/1277 | |||
| // eslint-disable-next-line consistent-return | |||
There was a problem hiding this comment.
TL;DR typescript-eslint/typescript-eslint#1277 core eslint consistent-return is not type aware, and typescript-eslint has not implemented an extension to avoid this issue (functions that return Promise<void> are not honoured in terms of return type)
There was a problem hiding this comment.
The clear comment about the reason here is really nice 👍
| return processMiddleware( | ||
| listenerMiddleware, | ||
| listenerArgs as AnyMiddlewareArgs, | ||
| if (listener === undefined) { |
There was a problem hiding this comment.
In this part, we have to explicitly, instead of implicitly, return undefined in the case the listener is undefined. I like to nest/indent the exceptional case and return right away, which allows me to do further processing below this conditional nested one indent less. So that's what I did here. 🤷
filmaj
changed the title
| @@ -695,6 +695,8 @@ export default class App { | |||
| } catch (error) { | |||
| this.logger.warn('Authorization of incoming event did not succeed. No listeners will be called.'); | |||
| error.code = 'slack_bolt_authorization_error'; | |||
| // disabling due to https://github.com/typescript-eslint/typescript-eslint/issues/1277 | |||
| // eslint-disable-next-line consistent-return | |||
There was a problem hiding this comment.
The clear comment about the reason here is really nice 👍
| return processMiddleware( | ||
| listenerMiddleware, | ||
| listenerArgs as AnyMiddlewareArgs, | ||
| if (listener === undefined) { |
| return reject(new ReceiverInconsistentStateError('The receiver cannot be stopped because it was not started.')); | ||
| } | ||
| this.server.close((error) => { | ||
| this.server?.close((error) => { |
There was a problem hiding this comment.
Hm. I assume ? was introduced because it no longer connects the check done above with the logic that follows. Can that addition be avoided if we added an else with the alternative path inside?
There was a problem hiding this comment.
@misscoded so if I remove the ?, when compiling the code (and, in particular, when trying to run the tests via npm run mocha), I get this:
➜ npm run mocha
> @slack/bolt@3.5.0 mocha /Users/fmaj/src/bolt-js
> TS_NODE_PROJECT=tsconfig.json nyc mocha --config .mocharc.json "src/**/*.spec.ts"
/Users/fmaj/src/bolt-js/node_modules/ts-node/src/index.ts:434
return new TSError(diagnosticText, diagnosticCodes)
^
TSError: ⨯ Unable to compile TypeScript:
src/receivers/ExpressReceiver.ts:342:7 - error TS2532: Object is possibly 'undefined'.
342 this.server.close((error) => {
... which is silly, since we have a check for undefined right before, but I guess because of the intervening Promise function scope, the TS compiler can't assume that the two scopes are executed in the same context / immediately after one another? Not sure.
If anyone has suggestions how to avoid this, I'd love to learn!
There was a problem hiding this comment.
Hm.. ok! No suggestions here at the moment
| return reject(new ReceiverInconsistentStateError('The receiver cannot be stopped because it was not started.')); | ||
| } | ||
| this.server.close((error) => { | ||
| this.server?.close((error) => { |
There was a problem hiding this comment.
Same comment/question as stop() in the ExpressReceiver.ts
filmaj
force-pushed
the
consistent-return-fixes
branch
from
4c4fe5c
to
93dbf74
Compare
Summary
This is a PR that should be merged into #1024 and incrementally addresses #842.
This PR is a separate approach to #1049: instead of turning off the
consistent-returnrule, here is my attempt at adhering to the rule.Impact
Before
After
Requirements (place an
xin each[ ])