chore: type-aware lint, and the four things it found - #34
Merged
Conversation
Four rules, on src only: no-floating-promises, no-misused-promises, await-thenable, require-await. The broad recommendedTypeChecked preset is deliberately not used -- most of it duplicates what strict already enforces, at the cost of a much slower lint and a backlog of style findings. Five errors, of which one is a bug and two are traps: - express/captcha.ts was an async RequestHandler. Express 4 does not catch a rejected promise from a handler, so anything thrown inside endpoints.handle() -- crypto, JSON, a store -- left the request hanging until the client timed out, and logged an unhandled rejection instead of returning 500. Now synchronous, with the rejection routed to next(). - invisible.ts attached an ASYNC submit listener. e.preventDefault() only works because nothing had awaited yet; once the handler yields the browser has already submitted and cancelling is a no-op. It worked, but it was one added `await` away from silently breaking every protected form, with no test that would notice. The async half is now its own method, kicked off after the cancel. - environment.ts let audioCtx.close() float. The surrounding try/catch does not cover it, so a rejection surfaced as an unhandled rejection in the user's console. - violation-reporter's flush timer and the fastify plugin signature are both fine as they were; they now say so rather than reading as oversights. Closes WebDecoy/app#738
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #738.
Four type-aware rules on
src(not tests):no-floating-promises,no-misused-promises,await-thenable,require-await.The broad
recommendedTypeCheckedpreset is deliberately not used. Most of it duplicates whatstrictalready enforces, at the cost of a much slower lint and a large backlog of findings that are style rather than defects. These four catch thingstsccannot.It found five errors. One is a bug, two are traps, two are fine and now say so.
The bug:
express/captcha.tshung on any throwExpress 4 does not catch a rejected promise from a handler. Anything thrown inside
endpoints.handle()— crypto, JSON parsing, a store access — left the request hanging until the client timed out, and logged an unhandled rejection instead of returning a 500. Now synchronous, with the rejection routed tonext()explicitly.The trap: an async
submitlistenerThis worked —
preventDefault()is reached before the firstawait, so the cancel lands. But it only works for that reason, and it was one addedawaitabove line 124 away from silently breaking every protected form: once the handler yields, the browser has already submitted and cancelling is a no-op. Nothing in the test suite would have caught it.The listener is now synchronous and the async half is its own method, kicked off with
voidafter the cancel. The invariant is in a comment rather than in someone's memory.The trap: a floating
audioCtx.close()Inside a
trywhosecatchdoes not cover it — the promise escapes the block, so a rejection surfaced as an unhandled rejection in the user's console.voidalone wouldn't fix that; it needs the.catch().The two that were fine
violation-reporter's flush timer (flush catches everything internally and never rejects) and the fastify plugin'sasyncsignature (that's the plugin contract). Both now carry avoid/disable and a sentence saying why, so they read as decisions rather than oversights. A timer whose callback rejects keeps firing and adds an unhandled rejection every tick — worth stating even when it isn't happening.Verification
0 errors across all six packages. Warning budgets unchanged (9/25/10/0/2/0). 446 tests pass. Lint runtime went from ~0.5s to ~4s for the whole workspace, which is why the rule set stays small.