Skip to content

fix(service-error-classification): add TypeError as transient error #1574

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

Merged
merged 3 commits into from
May 2, 2025

Conversation

siddsriv
Copy link
Contributor

@siddsriv siddsriv commented May 1, 2025

Issue #, if available:
Internal JS-5890

Description of changes:
fetch API surfaces connectivity errors as TypeErrors, example: TypeError: Failed to fetch in Chrome.

For browsers, we should retry for connectivity issues (offline), this is a fix to retry on TypeErrors then. Since we don't want to retry for TypeErrors that aren't network errors, I've paired this check with the actual error messages that different browsers surface (not an exhaustive list, but covers some major ones).

Fetch has an open issue about this: whatwg/fetch#526

additional resources


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@siddsriv siddsriv requested a review from a team as a code owner May 1, 2025 23:21
@siddsriv siddsriv force-pushed the browser-retries branch 2 times, most recently from 290d3cb to d246976 Compare May 2, 2025 03:12
@siddsriv siddsriv force-pushed the browser-retries branch from d246976 to e2730c2 Compare May 2, 2025 03:13
return false;
}

return errorMessages.has(error.message);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

doing a check for messages as well to avoid unintended TypeErrors from being retried

* @internal
*/
export const isBrowserNetworkError = (error: SdkError) => {
const errorMessages = new Set([
Copy link
Contributor Author

@siddsriv siddsriv May 2, 2025

Choose a reason for hiding this comment

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

this constant could be placed somewhere else, like the constants file in the same dir. I'd like this here since:

  1. these are not as strong a check as error codes or status codes, so I want to keep them away from other constants
  2. this function and its hacky message checks will be easily(-ier?) visible in the future if someone is taking a look at transient errors.

@siddsriv siddsriv force-pushed the browser-retries branch from e2730c2 to 18e37df Compare May 2, 2025 03:31
@siddsriv siddsriv force-pushed the browser-retries branch from 18e37df to 988b397 Compare May 2, 2025 03:38
@siddsriv siddsriv merged commit 89bde09 into smithy-lang:main May 2, 2025
10 checks passed
@siddsriv siddsriv deleted the browser-retries branch May 2, 2025 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants