fix: request download and upload not support responseReturn
#5456
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/modules/uploader.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 WalkthroughThis pull request introduces comprehensive modifications to the request module's download and upload functionality. The changes primarily focus on enhancing the Changes
Assessment against linked issues
Possibly related PRs
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
🔭 Outside diff range comments (1)
packages/effects/request/src/request-client/modules/downloader.test.ts (1)
Line range hint
23-35: Add test coverage for different responseReturn values.The test suite should verify behavior with both 'raw' and 'body' responseReturn values.
it('should download a file and return a Blob', async () => { const url = 'https://example.com/file'; const mockBlob = new Blob(['file content'], { type: 'text/plain' }); const mockResponse: Blob = mockBlob; mockAxiosInstance.get.mockResolvedValueOnce(mockResponse); const result = await fileDownloader.download(url); expect(result).toBeInstanceOf(Blob); expect(result).toEqual(mockBlob); expect(mockAxiosInstance.get).toHaveBeenCalledWith(url, { responseType: 'blob', responseReturn: 'body', }); }); +it('should return raw response when responseReturn is raw', async () => { + const url = 'https://example.com/file'; + const mockBlob = new Blob(['file content'], { type: 'text/plain' }); + const mockResponse = { data: mockBlob, headers: { 'content-type': 'text/plain' } }; + + mockAxiosInstance.get.mockResolvedValueOnce(mockResponse); + + const result = await fileDownloader.download(url, { responseReturn: 'raw' }); + + expect(result).toEqual(mockResponse); + expect(mockAxiosInstance.get).toHaveBeenCalledWith(url, { + responseType: 'blob', + responseReturn: 'raw', + }); +});
🧹 Nitpick comments (8)
packages/effects/request/src/request-client/modules/uploader.ts (1)
1-4: Consider completing the transition from Axios types.The file still uses
AxiosResponsewhile transitioning toRequestClientConfig. Based on the PR objectives regardingresponseReturnsupport, consider creating and using a custom response type that aligns with theresponseReturnconfiguration.playground/src/api/examples/download.ts (2)
9-13: RenamedownloadFile1for clarityConsider renaming
downloadFile1to a more descriptive name likedownloadBlobordownloadFileAsBlobto clearly indicate that the function downloads a file and returns aBlob. This improves code readability and maintainability.
19-26: RenamedownloadFile2to reflect its functionalitySimilarly, renaming
downloadFile2todownloadFileWithResponseordownloadFileRawResponsewould make it clearer that this function returns the fullRequestResponse<Blob>, including headers and status.packages/effects/request/src/request-client/types.ts (1)
10-12: Clarify usage ofresponseReturnin different contextsThe
responseReturnoption includes'data','body', and'raw'. However, in theDownloadRequestConfigused by thedownloadmethod, only'body'and'raw'are allowed. To prevent confusion, consider adding comments or documentation to explain that the'data'option is not supported for download operations.playground/src/views/demos/features/file-download/index.vue (3)
20-24: Add error handling to getBlob function.The function should handle potential download failures to provide feedback to users.
function getBlob() { downloadFile1().then((res) => { downloadResult.value = `获取Blob成功，长度：${res.size}`; + }).catch((error) => { + downloadResult.value = `获取Blob失败：${error.message}`; }); }
26-30: Add error handling to getResponse function.The function should handle potential download failures to provide feedback to users.
function getResponse() { downloadFile2().then((res) => { downloadResult.value = `获取Response成功，headers：${JSON.stringify(res.headers)},长度：${res.data.size}`; + }).catch((error) => { + downloadResult.value = `获取Response失败：${error.message}`; }); }
91-98: Consider adding loading states to download buttons.The UI should indicate when downloads are in progress to improve user experience.
<Card class="my-5" title="Request download"> - <Button type="primary" @click="getBlob"> 获取Blob </Button> - <Button type="primary" class="ml-4" @click="getResponse"> + <Button type="primary" :loading="isLoading" @click="getBlob"> 获取Blob </Button> + <Button type="primary" class="ml-4" :loading="isLoading" @click="getResponse"> 获取Response </Button> <div class="mt-4">{{ downloadResult }}</div> </Card>packages/effects/request/src/request-client/preset-interceptors.ts (1)
28-32: Consider adding type guards for responseReturn values.Add runtime checks to ensure responseReturn values are valid.
+const isValidResponseReturn = (value: unknown): value is 'raw' | 'body' => { + return value === 'raw' || value === 'body'; +}; if (config.responseReturn === 'raw') { return response; } if (status >= 200 && status < 400) { + if (config.responseReturn && !isValidResponseReturn(config.responseReturn)) { + throw new Error(`Invalid responseReturn value: ${config.responseReturn}`); + } if (config.responseReturn === 'body') { return responseData; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/effects/request/src/request-client/modules/downloader.test.ts(2 hunks)packages/effects/request/src/request-client/modules/downloader.ts(1 hunks)packages/effects/request/src/request-client/modules/uploader.ts(2 hunks)packages/effects/request/src/request-client/preset-interceptors.ts(1 hunks)packages/effects/request/src/request-client/types.ts(1 hunks)playground/src/api/examples/download.ts(1 hunks)playground/src/views/demos/features/file-download/index.vue(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test (windows-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: Lint (ubuntu-latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: post-update (windows-latest)
- GitHub Check: post-update (ubuntu-latest)
🔇 Additional comments (3)
packages/effects/request/src/request-client/modules/uploader.ts (1)
Line range hint
24-30: Ensure type-safe header merging withRequestClientConfig.The header merging logic might need type guards or proper type assertions when using the new
RequestClientConfigtype. Also, consider documenting that the content type will always be set to 'multipart/form-data' regardless of the input config.Let's verify the type definition:
✅ Verification successful
The header merging implementation is type-safe and appropriate for file uploads.
The
RequestClientConfigtype extendsAxiosRequestConfig, ensuring type safety for header operations. SettingContent-Typeto'multipart/form-data'is the correct default for file uploads, while still allowing header customization through the config parameter if needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if RequestClientConfig properly types the headers property ast-grep --pattern 'type RequestClientConfig = { $$$ headers?: $_ $$$ }'Length of output: 80
Script:
#!/bin/bash # Find RequestClientConfig definition or imports rg "RequestClientConfig" -A 3 -B 3 # Try alternative ast-grep pattern for type/interface ast-grep --pattern 'interface RequestClientConfig { $$$ }' # Check for type imports rg "import.*RequestClientConfig" --type tsLength of output: 8495
packages/effects/request/src/request-client/modules/downloader.ts (2)
25-28: Method signature and return type updated properlyThe updated method signature
public async download<T = Blob>(...)and the return typePromise<T>enhance flexibility by allowing different response types. This change aligns well with the updatedDownloadRequestConfig.
29-34: EnsureresponseTypeis always set to'blob'The
finalConfigcorrectly forcesresponseTypeto'blob'after spreading...config, ensuring that the response is always aBlob. This is essential for file downloads.
packages/effects/request/src/request-client/modules/uploader.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/effects/request/src/request-client/modules/uploader.ts (1)
11-15: 🛠️ Refactor suggestionEnhance type safety for
responseReturnconfiguration.While the generic type parameter is a good start, we should:
- Constrain the return type based on
responseReturnvalue- Add type validation for supported
responseReturnvaluesConsider updating the signature to:
public async upload<T = any, R = T>( url: string, data: Record<string, any> & { file: Blob | File }, config?: RequestClientConfig & { responseReturn?: 'data' | 'body' | 'raw' } ): Promise<R extends Response<T> ? Response<T> : T>
🧹 Nitpick comments (1)
packages/effects/request/src/request-client/modules/uploader.ts (1)
Line range hint
22-28: Add validation forresponseReturnconfiguration.The configuration merging looks good, but we should validate the
responseReturnvalue to ensure it's one of the supported options ('data', 'body', 'raw').Consider adding:
const finalConfig: RequestClientConfig = { ...config, headers: { 'Content-Type': 'multipart/form-data', ...config?.headers, }, }; if (finalConfig.responseReturn && !['data', 'body', 'raw'].includes(finalConfig.responseReturn)) { throw new Error('Invalid responseReturn value. Supported values are: data, body, raw'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/effects/request/src/request-client/modules/uploader.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test (windows-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: post-update (windows-latest)
- GitHub Check: Lint (ubuntu-latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: post-update (ubuntu-latest)
🔇 Additional comments (1)
packages/effects/request/src/request-client/modules/uploader.ts (1)
1-3: LGTM! Import changes align with the architectural direction.The switch from Axios-specific types to custom
RequestClientConfigtype is consistent with the goal of supportingresponseReturnconfiguration.
Description
修复requestClient的download和upload方法没有支持
responseReturn配置的问题：responseReturn只有在设为data时才会解构body部分并检查code是否代表成功。设为body时不会解构body部分，因此也不需要检查code是否成功（由调用方检查）fix #5455
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
Release Notes
New Features
Improvements
Documentation