perf: request support to set how to return response #5436
Conversation
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/effects/request/src/request-client/request-client.test.tsOops! Something went wrong! :( ESLint: 9.17.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/node_modules/@vben/eslint-config/dist/index.mjs' imported from /eslint.config.mjs WalkthroughThe pull request introduces modifications to the request handling mechanism across multiple files in the Vben Admin project. The primary changes involve updating the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Finishing Touches
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: 2
🧹 Nitpick comments (5)
packages/effects/request/src/request-client/types.ts (1)
8-15: Translate comments to English for better international collaboration.The comments explaining the
responseReturnoptions should be in English. Here's the suggested translation:- /** 响应数据的返回方式。 - * raw: 原始的AxiosResponse，包括headers、status等。 - * body: 返回响应数据的BODY部分。 - * data: 解构响应的BODY数据，只返回其中的data节点数据(默认方式)。 - */ + /** Response data return format. + * raw: Original AxiosResponse, including headers, status, etc. + * body: Returns the BODY part of the response. + * data: Destructures the BODY data, returns only the data node (default). + */apps/web-antd/src/api/request.ts (1)
23-25: Consider documenting the new options usage.The function now accepts options but lacks documentation about the available configuration, especially the new
responseReturnoption.+/** + * Creates a configured request client + * @param baseURL - The base URL for API requests + * @param options - Optional client configuration + * @param options.responseReturn - Response format ('raw' | 'body' | 'data') + * @returns Configured RequestClient instance + */ function createRequestClient(baseURL: string, options?: RequestClientOptions) {playground/src/api/request.ts (1)
Line range hint
97-99: Consider configuring exported client instances.The exported
requestClientandbaseRequestClientinstances don't specify theresponseReturnoption. Consider setting a default response format to maintain consistent behavior across the application.-export const requestClient = createRequestClient(apiURL); +export const requestClient = createRequestClient(apiURL, { responseReturn: 'data' }); -export const baseRequestClient = new RequestClient({ baseURL: apiURL }); +export const baseRequestClient = new RequestClient({ + baseURL: apiURL, + responseReturn: 'data' +});packages/effects/request/src/request-client/request-client.ts (1)
56-72: Consider adding type validation and documentation for response types.To improve the robustness of the new feature:
- Add TypeScript type validation for
responseReturnvalues- Add JSDoc documentation explaining the response type options
Consider adding these type definitions:
type ResponseReturnType = 'raw' | 'body' | 'data'; interface RequestClientConfig extends AxiosRequestConfig { responseReturn?: ResponseReturnType; }And update the constructor documentation:
/** * Creates an instance of RequestClient * @param options - Configuration options * @param options.responseReturn - Controls response format: * - 'raw': Returns complete AxiosResponse * - 'body': Returns response body * - 'data': Returns data node from body (default) */ constructor(options: RequestClientOptions = {}) {apps/web-naive/src/api/request.ts (1)
22-25: Consider documenting the new options and their impact.While the implementation is solid, consider:
- Adding JSDoc comments to document the new options parameter and its effects
- Updating relevant API documentation to explain the
responseReturnoption- Providing migration guides for consumers who relied on the removed data structure validation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web-antd/src/api/request.ts(2 hunks)apps/web-ele/src/api/request.ts(2 hunks)apps/web-naive/src/api/request.ts(2 hunks)packages/effects/request/src/request-client/request-client.ts(5 hunks)packages/effects/request/src/request-client/types.ts(2 hunks)playground/src/api/request.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Lint (windows-latest)
- GitHub Check: Lint (ubuntu-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: post-update (windows-latest)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: post-update (ubuntu-latest)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
packages/effects/request/src/request-client/types.ts (3)
16-20: LGTM! Well-structured type definitions.The types are properly defined with correct extension of Axios types and good use of generics.
28-28: LGTM! Good type composition.The type correctly extends CreateAxiosDefaults with ExtendOptions, providing a clean way to configure the request client.
32-35: LGTM! Robust type definitions for interceptors.The interceptor types are well-defined with proper handling of ExtendOptions and async operations.
Also applies to: 41-42
apps/web-antd/src/api/request.ts (1)
23-25: Verify the impact of removing the response interceptor.The removal of the response data structure validation interceptor changes how responses are processed. Ensure that all API consumers are updated to handle the raw response format correctly.
Let's check for direct response data access patterns that might break:
apps/web-ele/src/api/request.ts (2)
4-4: LGTM! Import statement updated correctly.The import statement has been updated to include
RequestClientOptionswhich is needed for the new configuration options.
23-26: LGTM! Function signature updated correctly.The changes correctly implement the new options parameter:
- Function signature updated to accept optional RequestClientOptions
- Options are properly forwarded to RequestClient using spread operator
packages/effects/request/src/request-client/request-client.ts (2)
1-7: LGTM! Import statements updated correctly.The imports have been properly updated to include the necessary types for the new response handling functionality.
85-88: LGTM! Method signatures updated consistently.All HTTP method signatures have been properly updated to use
RequestClientConfiginstead ofAxiosRequestConfig, maintaining consistency across the API.Also applies to: 95-95, 105-105, 116-116, 124-127
apps/web-naive/src/api/request.ts (2)
4-4: LGTM! Type import added for new options parameter.The import of
RequestClientOptionstype is correctly added to support the enhanced function signature.
24-25: Verify the impact of removing data structure validation.The options spreading is implemented correctly, with baseURL properly preserved. However, the removal of the data structure validation interceptor (as noted in the AI summary) requires verification:
- Ensure all API consumers are prepared to handle varied response structures
- Confirm this aligns with the new
responseReturnoption behaviorLet's analyze the impact:
✅ Verification successful
✓ Data structure validation removal verified as safe
The codebase analysis shows no rigid response structure assumptions in consuming applications, making the removal of the validation interceptor safe and aligned with the flexible
responseReturnoption design.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct RequestClient usage to identify affected consumers ast-grep --pattern 'new RequestClient($$$)' # Search for response data structure assumptions rg -g '*.{ts,tsx,vue}' -A 3 'response\.(data|body)'Length of output: 3767
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/effects/request/src/request-client/request-client.ts (1)
125-128: Enhance error handling in request method.Consider adding more context to errors for better debugging.
public async request<T>( url: string, config: RequestClientConfig, ): Promise<T> { try { const response: AxiosResponse<T> = await this.instance({ url, ...config, }); return response as T; } catch (error: any) { - throw error.response ? error.response.data : error; + const errorData = error.response?.data; + throw { + ...errorData, + url, + method: config.method, + message: errorData?.message || error.message, + status: error.response?.status + }; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web-antd/src/api/request.ts(3 hunks)apps/web-ele/src/api/request.ts(3 hunks)apps/web-naive/src/api/request.ts(3 hunks)packages/effects/request/src/request-client/request-client.ts(6 hunks)packages/effects/request/src/request-client/types.ts(2 hunks)playground/src/api/request.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/web-antd/src/api/request.ts
- apps/web-ele/src/api/request.ts
- packages/effects/request/src/request-client/types.ts
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test (windows-latest)
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: Lint (ubuntu-latest)
- GitHub Check: post-update (windows-latest)
- GitHub Check: post-update (ubuntu-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
packages/effects/request/src/request-client/request-client.ts (4)
1-7: LGTM! Import statements updated to support custom request configuration.The changes properly introduce the new types needed for configurable response returns.
36-43: LGTM! Default configuration properly initialized.The default
responseReturn: 'raw'ensures backward compatibility while enabling customization through options.
86-89: LGTM! Method signatures properly updated.The HTTP method implementations consistently use the new
RequestClientConfigtype.Also applies to: 96-96, 106-106, 117-117
57-73:⚠️ Potential issueImprove response interceptor implementation.
The current implementation has several potential issues:
- Missing type checking for responseData structure
- Unsafe destructuring of
codeanddata- Circular reference in error object
Consider this safer implementation:
this.addResponseInterceptor<HttpResponse>({ fulfilled: (response) => { const { config, data: responseData, status } = response; if (config.responseReturn === 'raw') { return response; } - const { code, data } = responseData; + // Safe handling of responseData + if (typeof responseData !== 'object' || responseData === null) { + return config.responseReturn === 'body' ? responseData : responseData; + } + + const code = 'code' in responseData ? responseData.code : undefined; + const data = 'data' in responseData ? responseData.data : responseData; if (status >= 200 && status < 400 && code === 0) { return config.responseReturn === 'body' ? responseData : data; } - throw Object.assign({}, response, { response }); + // Avoid circular reference + throw { + status, + data: responseData, + message: responseData?.message || 'Request failed' + }; }, });Likely invalid or redundant comment.
apps/web-naive/src/api/request.ts (1)
22-24: LGTM! Request client properly configured.The implementation correctly:
- Updates createRequestClient to support options
- Sets responseReturn to 'data' as default
Also applies to: 98-100
playground/src/api/request.ts (1)
23-25: LGTM! Implementation consistent with web-naive.The changes mirror those in web-naive/src/api/request.ts, maintaining consistency across the codebase.
Also applies to: 99-101
mynetfan
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/effects/request/src/request-client/request-client.test.ts (1)
51-72: LGTM! Well-structured test coverage for the new feature.The test case effectively validates all three response types ('raw', 'body', 'data') with clear assertions and consistent mock data.
Consider adding edge cases for comprehensive testing.
While the happy path is well covered, consider adding test cases for:
- Empty response data
- Missing or null fields in the response
- Invalid responseReturn option values
Example test case:
it('should handle edge cases for different response types', async () => { const emptyMockData = { code: 0, msg: '', data: null }; mock.onGet('/test/empty').reply(200, emptyMockData); // Test with null data const responseData = await requestClient.get('/test/empty', { responseReturn: 'data', }); expect(responseData).toBeNull(); // Test with invalid responseReturn option await expect( requestClient.get('/test/empty', { // @ts-expect-error - Testing invalid option responseReturn: 'invalid', }) ).rejects.toThrow(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/effects/request/src/request-client/request-client.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test (windows-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: post-update (windows-latest)
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: post-update (ubuntu-latest)
- GitHub Check: Lint (ubuntu-latest)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/effects/request/src/request-client/request-client.test.ts (1)
51-72: Verify consistent implementation across the codebase.Let's ensure the responseReturn option is consistently implemented across all request methods.
✅ Verification successful
Implementation verified as consistent and complete
The responseReturn option is properly implemented:
- Centralized response processing via interceptor ensures consistent behavior across all HTTP methods
- All three modes ('raw', 'body', 'data') are handled correctly
- Default configuration and usage patterns are consistent across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if all HTTP methods support the responseReturn option consistently # Search for request method implementations echo "Searching for request method implementations..." ast-grep --pattern 'class $_ { $$$ (get|post|put|delete)($_) { $$$ } $$$ }' # Search for responseReturn option usage echo "Checking responseReturn option usage..." rg -A 2 'responseReturn:'Length of output: 1956
Script:
#!/bin/bash # Search for the implementation of HTTP methods and response processing echo "Searching for HTTP method implementations..." ast-grep --pattern 'async $method($$$) { $$$ }' packages/effects/request/src/request-client/request-client.ts echo "\nSearching for response processing..." rg -A 10 'processResponse' packages/effects/request/src/request-client/request-client.tsLength of output: 388
Script:
#!/bin/bash # First verify the file exists and show its content echo "Checking file content..." cat packages/effects/request/src/request-client/request-client.ts echo "\nSearching for method implementations with broader pattern..." ast-grep --pattern 'class RequestClient { $$$ }'Length of output: 15176
Description
为 RequestClient 添加额外的选项。可以在实例化时、调用具体的请求方法时传入这些额外的选项来改变request的一些表现。
目前第一个新增的选项为
responseReturn，用于设置返回的数据类型，取值和说明如下：close #5432
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
Improvements
Changes