fix: changed return to match shape of error. #2744
Conversation
MichaelUnkey
requested review from
perkinsjr,
chronark,
mcstepp and
ogzhanolguncu
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThe changes in this pull request focus on modifying the error handling logic within the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/api/src/client.ts (1)
Line range hint
200-211: Improve type safety in error handlingThe use of
@ts-ignorecomments suggests underlying type system issues that should be addressed for better maintainability and type safety.Consider refactoring to improve type safety:
+type FetchError = Error & { message: string }; + return { error: { - // @ts-ignore code: "FETCH_ERROR", - // @ts-ignore I don't understand why `err` is `never` - message: err?.message ?? "No response", + message: (err as FetchError)?.message ?? "No response", docs: "https://developer.mozilla.org/en-US/docs/Web/API/fetch", requestId: "N/A", }, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/api/src/client.ts(1 hunks)
🔇 Additional comments (2)
packages/api/src/client.ts (2)
Line range hint 198-211: LGTM with suggestions for improvement
The changes successfully address the PR objective of matching error shapes by simplifying the error handling logic. While the implementation works, consider the suggested improvements for better type safety and error validation to make the code more robust.
198-198: Consider adding runtime validation for error response structure
While directly returning the response as ErrorResponse simplifies the code, it assumes the response will always match the expected error structure. Consider adding runtime validation to ensure type safety and prevent exposing unexpected error formats to clients.
Consider adding validation:
- return (await res.json()) as ErrorResponse;
+ const response = await res.json();
+ if (!response?.error?.code || !response?.error?.message) {
+ return {
+ error: {
+ code: "INVALID_ERROR_RESPONSE",
+ message: "Received invalid error response format",
+ docs: "https://docs.unkey.dev/errors",
+ requestId: response?.error?.requestId ?? "N/A"
+ }
+ };
+ }
+ return response as ErrorResponse;
What does this PR do?
Fixes # (issue)
ENG-1491
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Returns Correct errors
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit