Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ESLint Config Migration: Fixing consistent-return linter warnings #1075

Merged
merged 1 commit into from
Aug 25, 2021
Merged

ESLint Config Migration: Fixing consistent-return linter warnings #1075

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 19 additions & 14 deletions src/App.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The clear comment about the reason here is really nice 👍

return this.handleError(error);
}

Expand Down Expand Up @@ -860,22 +862,23 @@ export default class App {
// Don't process the last item in the listenerMiddleware array - it shouldn't get a next fn
const listener = listenerMiddleware.pop();

if (listener !== undefined) {
return processMiddleware(
listenerMiddleware,
listenerArgs as AnyMiddlewareArgs,
if (listener === undefined) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🤷

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine.

return undefined;
}
return processMiddleware(
listenerMiddleware,
listenerArgs as AnyMiddlewareArgs,
context,
client,
this.logger,
// When the listener middleware chain is done processing, call the listener without a next fn
async () => listener({
...(listenerArgs as AnyMiddlewareArgs),
context,
client,
this.logger,
// When the listener middleware chain is done processing, call the listener without a next fn
async () => listener({
...(listenerArgs as AnyMiddlewareArgs),
context,
client,
logger: this.logger,
}),
);
}
logger: this.logger,
}),
);
});

const settledListenerResults = await allSettled(listenerResults);
Expand All @@ -890,6 +893,8 @@ export default class App {
},
);
} catch (error) {
// disabling due to https://github.com/typescript-eslint/typescript-eslint/issues/1277
// eslint-disable-next-line consistent-return
return this.handleError(error);
}
}
Expand Down
12 changes: 6 additions & 6 deletions src/receivers/ExpressReceiver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,25 +327,25 @@ export default class ExpressReceiver implements Receiver {
return reject(new ReceiverInconsistentStateError(missingServerErrorDescription));
}

resolve(this.server);
return resolve(this.server);
});
});
}

// TODO: the arguments should be defined as the arguments to close() (which happen to be none), but for sake of
// generic types
public stop(): Promise<void> {
if (this.server === undefined) {
return Promise.reject(new ReceiverInconsistentStateError('The receiver cannot be stopped because it was not started.'));
}
return new Promise((resolve, reject) => {
if (this.server === undefined) {
return reject(new ReceiverInconsistentStateError('The receiver cannot be stopped because it was not started.'));
}
this.server.close((error) => {
this.server?.close((error) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm.. ok! No suggestions here at the moment

if (error !== undefined) {
return reject(error);
}

this.server = undefined;
resolve();
return resolve();
});
});
}
Expand Down
12 changes: 6 additions & 6 deletions src/receivers/HTTPReceiver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,25 +242,25 @@ export default class HTTPReceiver implements Receiver {
return reject(new ReceiverInconsistentStateError(missingServerErrorDescription));
}

resolve(this.server);
return resolve(this.server);
});
});
}

// TODO: the arguments should be defined as the arguments to close() (which happen to be none), but for sake of
// generic types
public stop(): Promise<void> {
if (this.server === undefined) {
return Promise.reject(new ReceiverInconsistentStateError('The receiver cannot be stopped because it was not started.'));
}
return new Promise((resolve, reject) => {
if (this.server === undefined) {
return reject(new ReceiverInconsistentStateError('The receiver cannot be stopped because it was not started.'));
}
this.server.close((error) => {
this.server?.close((error) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment/question as stop() in the ExpressReceiver.ts

if (error !== undefined) {
return reject(error);
}

this.server = undefined;
resolve();
return resolve();
});
});
}
Expand Down