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.

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

🪟 🔧 Cleaner error handling for authService's sendVerificationEmail #21763

Merged

Conversation

ambirdsall
Copy link
Contributor

@ambirdsall ambirdsall commented Jan 24, 2023

What

When I first extracted this error handling code from EmailVerificationHint.tsx in #21623 so the enrollment modal could take advantage of it too, I missed a basic structural fix (because the internal API defined in authService had an identical type signature as the upstream firebase/auth function it wrapped, it Seemed Obvious:tm: that it was appropriate to port its error-handling code unchanged). To wit: because it handled errors within the Promise, rather than via the Promise, success/error code paths were being defined in different scopes; the only way to invalidate a downstream .then which should only run on success was to reraise the error. This lead to arbitrary and ugly .catch()es getting tacked onto both usage sites just to avoid crashing the page from an already-handled error.

But, like: that's the whole reason Promise.prototype.catch exists, though.

Testing

Manually tested via the enrollment modal, behaves exactly as before in both error and success states.

@octavia-squidington-iv octavia-squidington-iv added the area/frontend Related to the Airbyte webapp label Jan 24, 2023
@ambirdsall ambirdsall marked this pull request as ready for review January 24, 2023 07:54
@ambirdsall ambirdsall requested a review from josephkmh January 24, 2023 07:54
@ambirdsall ambirdsall force-pushed the alex/improve-error-handling-for-sendEmailVerification branch from a48a060 to 4f09689 Compare January 26, 2023 22:50
Copy link
Contributor

@josephkmh josephkmh left a comment

Choose a reason for hiding this comment

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

Thanks for refactoring this! Explanation makes sense, and this is much cleaner 👍

@ambirdsall ambirdsall merged commit 819896e into master Jan 27, 2023
@ambirdsall ambirdsall deleted the alex/improve-error-handling-for-sendEmailVerification branch January 27, 2023 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants