perf: improved exception handling when request status code is 200 #4679
Conversation
|
WalkthroughThe pull request introduces modifications to the response handling logic in the Changes
Assessment against linked issues
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
apps/web-naive/src/api/request.ts (1)
76-81: Improved error handling, but consider further enhancementsThe changes address the core issue mentioned in #4667 by throwing a structured error object instead of a string-type error message. This allows for more detailed error information to be passed down the chain, enabling custom error handling based on the error code and message.
However, there are a few points to consider:
- The PR title mentions improving exception handling when the status code is 200, but there's no explicit check for this status code in the changed code.
- The error object structure could be more explicit. Consider structuring it like this for easier error handling downstream:
const error = { response, code, message: responseData.message || 'Unknown error' };
- It might be beneficial to add a comment explaining the rationale behind this change, especially mentioning how it addresses the issue with string-type error messages.
Would you like me to propose a more structured error object implementation?
apps/web-ele/src/api/request.ts (1)
77-82: Approve changes with a minor suggestion for improvementThe modifications to the response interceptor effectively address the issues mentioned in PR #4667 and align with the objectives of improving exception handling. By throwing an error object containing the entire response, you've enabled more robust error handling downstream.
However, for clarity and consistency, consider the following minor improvement:
- const error = { response }; - throw error; + throw new Error('Request failed', { cause: response });This change utilizes the Error object's
causeproperty, which is a standard way to attach additional information to errors. It maintains the ability to access the full response while providing a more structured error object.docs/src/guide/essentials/server.md (1)
Line range hint
91-106: Remove duplicate function definitions in POST/PUT request exampleThe POST/PUT request example contains duplicate function definitions for
saveUserApi. This redundancy might confuse readers. Consider consolidating these examples to show the different HTTP methods more clearly.Here's a suggested revision:
import { requestClient } from '#/api/request'; // POST example export async function createUserApi(user: UserInfo) { return requestClient.post<UserInfo>('/user', user); } // PUT example export async function updateUserApi(user: UserInfo) { return requestClient.put<UserInfo>(`/user/${user.id}`, user); } // Combined POST/PUT example export async function saveUserApi(user: UserInfo) { const url = user.id ? `/user/${user.id}` : '/user'; return requestClient.request<UserInfo>(url, { data: user, method: user.id ? 'PUT' : 'POST', }); }This revision clearly demonstrates the different ways to handle POST and PUT requests, including a combined approach.
docs/src/en/guide/essentials/server.md (3)
Line range hint
76-116: LGTM! Clear and concise API request examples.The added examples for GET, POST, PUT, and DELETE requests using the
requestClientare well-structured and provide clear guidance for developers. The use of TypeScript generics enhances type safety, which is a good practice.Consider adding a brief comment above each example to explain its purpose, e.g., "// Example of a GET request to fetch user information". This would further improve readability and understanding for developers new to the project.
236-242: Improved error handling aligns with PR objectives.The changes to the response processing and error handling in the
createRequestClientfunction effectively address the issues mentioned in PR #4667. By throwing an error object containing the entire response, you ensure that subsequent error handling processes receive structured error information rather than a string-type error message.Consider adding a custom error class to provide more context and maintain consistency in error handling across the application. For example:
class ApiError extends Error { constructor(public response: any) { super('API Error'); this.name = 'ApiError'; } } // Then in the response interceptor: throw new ApiError(response);This approach would allow for more specific error handling and easier debugging.
Line range hint
301-337: Comprehensive guidance on refresh token configuration.The new section on refresh token configuration provides clear and detailed instructions for enabling and setting up the refresh token functionality. The example code for the
doRefreshTokenfunction is well-structured and consistent with the project's architecture.To enhance security, consider adding a note about securely storing the refresh token, possibly using HTTP-only cookies or secure storage mechanisms. Additionally, you might want to mention the importance of implementing token rotation or single-use refresh tokens to mitigate the risk of token theft.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- apps/web-antd/src/api/request.ts (1 hunks)
- apps/web-ele/src/api/request.ts (1 hunks)
- apps/web-naive/src/api/request.ts (1 hunks)
- docs/src/en/guide/essentials/server.md (1 hunks)
- docs/src/guide/essentials/server.md (1 hunks)
- playground/src/api/request.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (9)
playground/src/api/request.ts (3)
77-83: Summary of changes and their impactThe modifications in this file successfully address the issue described in #4667 and align with the PR objectives. The changes improve exception handling by:
- Simplifying the response data destructuring.
- Throwing an object containing the entire response, enabling more detailed error handling downstream.
These improvements allow for custom error handling based on error codes and messages, as intended. However, to ensure a smooth transition, please:
- Clarify how error messages will be handled without the
messagefield.- Verify that all parts of the codebase expecting the previous error format have been updated.
Overall, these changes represent a significant improvement in error handling capabilities.
82-83: Improved error handling aligns with PR objectives.This change addresses the issue described in #4667 by throwing an object containing the entire response instead of a formatted error message. This approach enables more detailed and custom error handling downstream, allowing access to error codes and messages as needed.
To ensure compatibility with existing error handlers, please verify that all parts of the codebase expecting the previous error format (
Error ${status}: ${msg}) have been updated accordingly. You can use the following script to search for potential locations that might need updating:#!/bin/bash # Search for usage of the old error format rg --type typescript 'Error.*:.*msg'
77-77: Clarify the impact of removing themessagefield.The
messagefield has been removed from the response data destructuring. While this simplifies the response handling, it may impact error reporting elsewhere in the code.Could you please clarify:
- How will error messages be handled without the
messagefield?- Are there any other parts of the codebase that rely on the
messagefield from the response?To verify the usage of the
messagefield, you can run the following script:✅ Verification successful
Removal of the
messagefield verified.No usage of the
messagefield fromresponseDatawas found in the codebase, ensuring that error reporting remains unaffected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of 'message' field from response data rg --type typescript 'const.*message.*=.*response(Data)?'Length of output: 248
Script:
#!/bin/bash # Search for usage of 'message' field from response data in TypeScript files rg 'const.*message.*=.*response(Data)?' --glob '*.ts'Length of output: 55
apps/web-ele/src/api/request.ts (1)
77-82: Verify error handling compatibility and update documentationWhile the changes improve the error handling in the response interceptor, it's important to ensure compatibility with the rest of the system:
Update
errorMessageResponseInterceptor:
TheerrorMessageResponseInterceptorshould be modified to handle the new error format. Currently, it expectserror?.response?.data, which should still work, but it might benefit from a more direct approach to accessing the response data.System-wide impact:
Any code that catches errors from this client will need to be updated to handle the new error format. Ensure that all error handling code throughout the application is reviewed and updated accordingly.Documentation:
Update the API documentation to reflect the new error handling behavior, ensuring that developers using this client are aware of the change in error format.To assist in identifying potential areas that need updating, you can run the following command to search for error handling related to this client:
This will help identify areas in the codebase where error handling is occurring, which may need to be updated to accommodate the new error format.
apps/web-antd/src/api/request.ts (1)
77-83: Improved error handling aligns with PR objectives.The changes to the response interceptor address the issue described in #4667 by throwing an error object containing the entire response. This approach allows for more flexible error handling downstream and ensures that a response object is returned via a Promise (by throwing it).
However, consider the following:
The change in error object structure might affect error handling in other parts of the application. Ensure that all error handling code is updated accordingly.
It might be beneficial to add a custom error class to provide more context and maintain consistency in error handling.
To verify the impact of these changes, please run the following script:
docs/src/guide/essentials/server.md (1)
Line range hint
1-383: Documentation updates align with code changes, minor improvements suggestedThe changes to the server interaction documentation accurately reflect the modifications made to the response interceptor. The new error handling mechanism addresses the issue raised in #4667 by providing more flexibility in downstream error processing.
Key points:
- The response interceptor now throws an error object containing the full response, allowing for more detailed error handling.
- The documentation has been updated to explain these changes and their implications.
Recommendations:
- Address the duplicate function definitions in the POST/PUT request example as suggested in the previous comment.
- Consider adding a note about the potential need to sanitize error objects before logging or displaying them to users, given that they now contain the full response.
Overall, these changes improve the error handling capabilities of the system and provide clearer documentation for developers. With the suggested minor improvements, this update will significantly enhance the project's robustness and maintainability.
docs/src/en/guide/essentials/server.md (3)
Line range hint
339-359: Clear guidance on data mocking practices.The updates to the data mocking section provide valuable clarification on the use of mock data in different environments. The emphasis on using real interfaces in production and the instructions for using Nitro in development are well-explained and align with best practices.
The inclusion of links to the Nitro documentation is helpful for developers who need more detailed information.
Line range hint
361-367: Useful instructions for disabling the mock service.The new section on disabling the mock service provides clear and concise instructions for developers who need to switch to real API endpoints during development. This addition enhances the flexibility of the development setup and aligns well with the project's documentation objectives.
The use of an environment variable for this configuration is a good practice, allowing for easy toggling of the mock service.
Line range hint
1-367: Comprehensive update to server interaction and data mocking documentation.The changes to this documentation file significantly improve the guidance provided for API configuration, request handling, and mock data processing. The updates effectively address the issues mentioned in PR #4667, particularly in the areas of exception handling and error object structure.
Key improvements include:
- Clear examples of API requests using different HTTP methods.
- Updated error handling in the request configuration to ensure structured error objects.
- Comprehensive guidance on refresh token configuration.
- Clarification on data mocking practices, emphasizing the use of real interfaces in production.
- Instructions for disabling the mock service when needed.
These changes enhance the overall quality and usefulness of the documentation, providing developers with clear and actionable information for working with the project's server interaction and data mocking features.
Description
fixed #4667
Type of change
Please delete options that are not relevant.
pnpm-lock.yamlunless you introduce a new test example.Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit
New Features
Documentation