fix(frontend): Use type-aware recommended ESLint rules #18662
Conversation
Twixes
force-pushed
the
no-floating-promises
branch
from
985e4e6 to
96b0a9f
Compare
|
Size Change: -60 B (0%) Total Size: 2.01 MB
|
| const response = await api.get( | ||
| `api/projects/${teamLogic.values.currentTeamId}/activity_log/important_changes` | ||
| )) as ChangesResponse | ||
| ) |
There was a problem hiding this comment.
I think get can take a type right? like api.get<ChangesResponse>(
There was a problem hiding this comment.
Good catch. This is fairly insignificant, because knowledge of this type isn't used in the function, and the return type is already specified because of the reducer's type – but better to retain the type info, so I made use of generics as you suggested
| @@ -104,7 +104,7 @@ export const activityLogLogic = kea<activityLogLogicType>([ | |||
| })), | |||
| listeners(({ actions }) => ({ | |||
| setPage: async (_, breakpoint) => { | |||
| await breakpoint() | |||
| breakpoint() | |||
There was a problem hiding this comment.
Is this not the opposite of what we want? 🤔
There was a problem hiding this comment.
This is actually a bit tricky and eslint gets it right, which is that breakpoints without a delay are non-promises (there's nothing asynchronous going on then) – as Marius explains below
| @@ -66,7 +66,7 @@ export const login2FALogic = kea<login2FALogicType>([ | |||
| : null, | |||
| }), | |||
| submit: async ({ token }, breakpoint) => { | |||
| await breakpoint() | |||
| breakpoint() | |||
There was a problem hiding this comment.
These ones confuse me totally as I thought the whole thing that started off the linting exercise was realizing we weren't awaiting the breakpoints?
There was a problem hiding this comment.
The await is only needed if you pass in an actual numeric value to wait. Otherwise the code executes synchronously and the await isn't needed. It should be just a no-op then and won't hurt, but it's effectively ignored.
Essentially:
function breakPoint(delay?: ms): void | Promise<void> {
if (delay > 0) {
await sleep(delay)
}
if (actionWasCalledAgain()) {
throw BreakpointException();
}
}
|
From what I can tell, things are working correctly, so will consider your feedback addressed @benjackwhite |
|
Hi folks - this is breaking our persons view for a large majority of our users.
|
Problem
In #18469 I noticed one class of real issue in our codebase, where we mistakenly didn't await
breakpoint(<delay>)in some Kea logics, which silently made the breakpoint a no-op – to the point of the logic not working as the developer intended at all.I realized: wow, how come ESLint didn't just flag this? Turns out it's perfectly capable of it, we just never turned the rule on. It's called
@typescript-eslint/no-floating-promises. So I added that.But then I realized, why isn't such a simple check just part of the
@typescript-eslint/recommendedset of rules (which we had enabled)? Well, for some reason there's@typescript-eslint/recommended… and there's@typescript-eslint/recommended-type-checked. The latter is a far more useful superset of the former, and does include@typescript-eslint/no-floating-promises.Changes
So I replaced
@typescript-eslint/recommendedwith@typescript-eslint/recommended-type-checked, and fixed all the errors that showed up. Around 300 of them. Mostly cases of missingawait/void, redundantasync, or unexpectedasync. And autofix handled redundant type assertions (e.g.const x = 'foo'; func(x as string);).How did you test this code?
pnpm lint:js