chore: resolve various eslint complaints #169

Sainan · 2024-05-05T14:39:18-07:00

Sainan commented

2024-05-05 14:39:18 -07:00

Some warnings remain, none of them particularly useful, but I've decided not to disable them. However, I did decide to disable @typescript-eslint/no-misused-promises due to basically only being noise, and removed the respective comments made because of it.

Some warnings remain, none of them particularly useful, but I've decided not to disable them. However, I did decide to disable `@typescript-eslint/no-misused-promises` due to basically only being noise, and removed the respective comments made because of it.

OrdisPrime commented

2024-05-05 14:55:06 -07:00

(Migrated from github.com)

Sadly express typescript typings don't support async function handlers, therefore we need to use that misused-promises ignore line frequently.
However, I would like to keep the setting enabled, as it is generally quite important to keep on.
Yes it means more noise (until the express situation is handled- there are solutions), but its an important setting to keep enabled.

Sadly express typescript typings don't support async function handlers, therefore we need to use that misused-promises ignore line frequently. However, I would like to keep the setting enabled, as it is generally quite important to keep on. Yes it means more noise (until the express situation is handled- there are solutions), but its an important setting to keep enabled.

Sainan commented

2024-05-05 14:56:17 -07:00

Is there any example where the @typescript-eslint/no-misused-promises lint has flagged a warning and it was actually useful?

Is there any example where the `@typescript-eslint/no-misused-promises` lint has flagged a warning and it was actually useful?

OrdisPrime commented

2024-05-05 15:09:02 -07:00

(Migrated from github.com)

Is there any example where the @typescript-eslint/no-misused-promises lint has flagged a warning and it was actually useful?

I had a quick look at ts-eslint and the examples there look like it would be useful.

There seems to be a half-decent "fix": casting the function handler to the correct type

accountRoutes.post('/endpoint', (async (req, res) => {
    // handler code here 
}) as RequestHandler);

Reference to the problem (express typings are not up2date):
git issue
git issue

> Is there any example where the `@typescript-eslint/no-misused-promises` lint has flagged a warning and it was actually useful? I had a quick look at [ts-eslint](https://typescript-eslint.io/rules/no-misused-promises/) and the examples there look like it would be useful. There seems to be a half-decent "fix": casting the function handler to the correct type ``` accountRoutes.post('/endpoint', (async (req, res) => { // handler code here }) as RequestHandler); ``` Reference to the problem (express typings are not up2date): [git issue](https://www.github.com/DefinitelyTyped/DefinitelyTyped/issues/50871) [git issue](https://ww.github.com/expressjs/express/issues/4892)

Sainan commented

2024-05-05 15:14:41 -07:00

Well, just looking at this project specifically, every time the warning was raised, it was either ignored, or a comment was added to suppress it. I feel like just disabling it project-wide is the best move until the upstream issue is resolved.

OrdisPrime commented

2024-05-05 15:21:05 -07:00

(Migrated from github.com)

Well, just looking at this project specifically, every time the warning was raised, it was either ignored, or a comment was added to suppress it. I feel like just disabling it project-wide is the best move until the upstream issue is resolved.

The upstream issue won't be solved in the near future it seems, based on how long this problem exists.
I'm heavily against turning off typescript checks- the whole reason of using typescript is that we get these warnings.

The comment to suppress these warnings is harmless, bugs resulting from code that is uncaught due to the check being disabled is dangerous.

Also, I would think that I've had the warning happen to me occasionally but I fixed the error, so that would of course make it useful.

I would happily cast every function handler as RequestHandler, then the problem is solved :)

> Well, just looking at this project specifically, every time the warning was raised, it was either ignored, or a comment was added to suppress it. I feel like just disabling it project-wide is the best move until the upstream issue is resolved. The upstream issue won't be solved in the near future it seems, based on how long this problem exists. I'm heavily against turning off typescript checks- the whole reason of using typescript is that we get these warnings. The comment to suppress these warnings is harmless, bugs resulting from code that is uncaught due to the check being disabled is dangerous. Also, I would think that I've had the warning happen to me occasionally but I fixed the error, so that would of course make it useful. I would happily cast every function handler as RequestHandler, then the problem is solved :)

Sainan commented

2024-05-05 15:31:31 -07:00

Well, I've reverted that change.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: OpenWF/SpaceNinjaServer#169