refactor: replace retry with internal async backoff retry loop#10663
refactor: replace retry with internal async backoff retry loop#10663joehan wants to merge 1 commit into
Conversation
### Description Removed the external third-party `retry` and `@types/retry` dependencies and replaced them with a native async retry loop with exponential backoff inside `src/apiv2.ts`. ### Scenarios Tested - Run all `src/apiv2.spec.ts` unit tests successfully. ### Sample Commands - `npx mocha src/apiv2.spec.ts`
There was a problem hiding this comment.
Code Review
This pull request removes the external retry dependency and replaces its usage in src/apiv2.ts with a custom retry loop using a while (true) block and a sleep utility. The review feedback suggests avoiding the use of any in catch blocks to comply with the repository's TypeScript guidelines, and improving error wrapping in FirebaseError to preserve the original stack trace and error details.
| } catch (thrown: any) { | ||
| const err = thrown instanceof Error ? thrown : new Error(thrown); |
There was a problem hiding this comment.
Avoid using any as an escape hatch per the TypeScript guidelines in the repository style guide. We should type the caught variable as unknown and use a safe conversion to Error to ensure type safety.
| } catch (thrown: any) { | |
| const err = thrown instanceof Error ? thrown : new Error(thrown); | |
| } catch (thrown: unknown) { | |
| const err = thrown instanceof Error ? thrown : new Error(String(thrown)); |
References
- Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. (link)
| } catch (err: unknown) { | ||
| throw err instanceof FirebaseError ? err : new FirebaseError(`${err}`); | ||
| } |
There was a problem hiding this comment.
When wrapping a generic caught error in a FirebaseError, using new FirebaseError(${err}) converts the error to a string (which includes the error name, e.g., "Error: message") and discards the original stack trace.
Instead, we should check if the caught error is an instance of Error and pass it as the original property to preserve the stack trace and original error details.
} catch (err: unknown) {
if (err instanceof FirebaseError) {
throw err;
}
if (err instanceof Error) {
throw new FirebaseError(err.message, { original: err });
}
throw new FirebaseError(String(err));
}| } finally { | ||
| // If we succeed or failed, clear the timeout. | ||
| if (reqTimeout) { | ||
| clearTimeout(reqTimeout); |
Description
Removed the external third-party
retryand@types/retrydependencies and replaced them with a native async retry loop with exponential backoff insidesrc/apiv2.ts.Scenarios Tested
src/apiv2.spec.tsunit tests successfully.Sample Commands
npx mocha src/apiv2.spec.ts