refactor: drop try/catch around client calls that cannot throw#98
Merged
Conversation
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.
What
Remove the
try/catchblocks that guarded client calls which can no longer throw, and document the few that are kept on purpose.Closes #89.
Removed
Six unreachable handlers, all wrapping nothing but client calls whose failures already arrive through
error:AuthProvider: thecatchinlogoutandlogoutAllSessionsLogin:register,sendMagicLink,sendPhoneOtp,sendEmailOtpPhoneRegistration: the innerverifyPhoneOTPhandlerVerifyMagicLink: the verify handlerKept, with a comment saying why
Not every
catchhere was dead, so these stay and now explain themselves rather than implying the client throws:Login,PhoneRegistration, andEmailRegistrationsubmit handlers, as backstops for genuinely unexpected errorsMagicLinkSentpoll, because a rejection inside a timer has no caller to surface itUntouched elsewhere, because they guard code that really does throw:
startAuthenticationandstartRegistration(a cancelled prompt),response.json()and the fetch promise in the result layer,localStoragein private mode,IntlinphoneInput, andstartOAuthLogininOAuthProviderButtons, which is a provider helper and therefore throws by design.logoutkeeps its guaranteelogoutandlogoutAllSessionsbecametry/finallyrather than losing the block entirely. Thecatchwas dead, but thefinallyis load-bearing: local auth state must be cleared even when the server call fails, or the UI keeps presenting a signed-in user whose session is gone. There is a comment on both, and the test added in #96 covers it.This exposed two tests that were passing for the wrong reason
logs out if token validation fails (bad response)andlogs out if token validation throwsboth mocked only the/users/mecall.validateTokenthen callslogout(), whose request was never mocked and resolvedundefined, which maderequestResultthrow aTypeErrorwhile readingresponse.ok. The oldcatchinlogoutswallowed it, so the tests passed while asserting less than they appeared to.With the
catchgone theTypeErrorsurfaced, so both mocks now provide the logout response as well. The assertions are unchanged and now actually exercise the path they describe.Worth noting the underlying detail:
requestResultassumes the request promise resolves to aResponse. That only breaks under a bad mock or a broken fetch shim, so I did not add speculative defence, but it is the reason those tests failed the moment the catch went away.Checks
npm run typecheck,npm run lint,npm run format:checkcleanLogin.tsx73.3% to 76.9% statements,PhoneRegistration.tsx89.4% to 91.5%,VerifyMagicLink.tsx93.9% to 95.7%, repo overall 88.9% to 89.7%