fix: 优化文件下载器方法 #6617
fix: 优化文件下载器方法 #6617
Conversation
|
WalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Suggested reviewers
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/effects/request/src/request-client/modules/downloader.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Test (ubuntu-latest)
packages/effects/request/src/request-client/modules/downloader.ts
[failure] 36-36: packages/effects/request/src/request-client/modules/downloader.test.ts > fileDownloader > should merge provided config with default config
TypeError: this.client.request is not a function
❯ FileDownloader.download packages/effects/request/src/request-client/modules/downloader.ts:36:40
❯ packages/effects/request/src/request-client/modules/downloader.test.ts:49:41
[failure] 36-36: packages/effects/request/src/request-client/modules/downloader.test.ts > fileDownloader > should download a file and return a Blob
TypeError: this.client.request is not a function
❯ FileDownloader.download packages/effects/request/src/request-client/modules/downloader.ts:36:40
❯ packages/effects/request/src/request-client/modules/downloader.test.ts:28:41
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: Lint (ubuntu-latest)
- GitHub Check: post-update (windows-latest)
- GitHub Check: post-update (ubuntu-latest)
🔇 Additional comments (1)
packages/effects/request/src/request-client/modules/downloader.ts (1)
29-34: Default method placement and binary response config validated
method: 'GET'is set before...config, allowing callers to override the HTTP methodresponseType: 'blob'is after...config, enforcing binary download responses- Verified that
RequestClientexposesget,post,put,delete, and a genericrequestmethod that forwards themethodfield to Axios
| ...config, | ||
| responseType: 'blob', | ||
| }; | ||
|
|
||
| const response = await this.client.get<T>(url, finalConfig); | ||
| const response = await this.client.request<T>(url, finalConfig); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Regression: switching to this.client.request breaks existing clients/mocks (tests failing).
Static analysis shows TypeError: this.client.request is not a function. The RequestClient in this codebase (and its tests) likely doesn’t expose request(url, config). This change breaks backward compatibility.
Proposed fix: add a safe dispatch that prefers request when available, otherwise falls back to get for GET and to method-specific calls for others.
Apply this diff to lines around the request invocation:
- const response = await this.client.request<T>(url, finalConfig);
-
- return response;
+ // Prefer a generic request if available; otherwise, dispatch to method-specific calls.
+ const method = (finalConfig.method || 'GET').toUpperCase();
+ const clientAny = this.client as any;
+ const lower = method.toLowerCase();
+
+ if (typeof clientAny.request === 'function') {
+ return await clientAny.request<T>(url, finalConfig);
+ }
+
+ // Fallbacks for common method signatures
+ if (method === 'GET' && typeof clientAny.get === 'function') {
+ return await clientAny.get<T>(url, finalConfig);
+ }
+
+ if (['POST', 'PUT', 'PATCH'].includes(method) && typeof clientAny[lower] === 'function') {
+ const { data, ...rest } = finalConfig as Record<string, any>;
+ return await clientAny[lower]<T>(url, data, rest);
+ }
+
+ if (['DELETE', 'HEAD', 'OPTIONS'].includes(method) && typeof clientAny[lower] === 'function') {
+ return await clientAny[lower]<T>(url, finalConfig);
+ }
+
+ throw new Error(`RequestClient does not support method "${method}"`);Alternatively (bigger change): add request(url, config) to RequestClient, update its implementations and tests to stub request.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const response = await this.client.request<T>(url, finalConfig); | |
| // Prefer a generic request if available; otherwise, dispatch to method-specific calls. | |
| const method = (finalConfig.method || 'GET').toUpperCase(); | |
| const clientAny = this.client as any; | |
| const lower = method.toLowerCase(); | |
| if (typeof clientAny.request === 'function') { | |
| return await clientAny.request<T>(url, finalConfig); | |
| } | |
| // Fallbacks for common method signatures | |
| if (method === 'GET' && typeof clientAny.get === 'function') { | |
| return await clientAny.get<T>(url, finalConfig); | |
| } | |
| if (['POST', 'PUT', 'PATCH'].includes(method) && typeof clientAny[lower] === 'function') { | |
| const { data, ...rest } = finalConfig as Record<string, any>; | |
| return await clientAny[lower]<T>(url, data, rest); | |
| } | |
| if (['DELETE', 'HEAD', 'OPTIONS'].includes(method) && typeof clientAny[lower] === 'function') { | |
| return await clientAny[lower]<T>(url, finalConfig); | |
| } | |
| throw new Error(`RequestClient does not support method "${method}"`); |
🧰 Tools
🪛 GitHub Check: Test (ubuntu-latest)
[failure] 36-36: packages/effects/request/src/request-client/modules/downloader.test.ts > fileDownloader > should merge provided config with default config
TypeError: this.client.request is not a function
❯ FileDownloader.download packages/effects/request/src/request-client/modules/downloader.ts:36:40
❯ packages/effects/request/src/request-client/modules/downloader.test.ts:49:41
[failure] 36-36: packages/effects/request/src/request-client/modules/downloader.test.ts > fileDownloader > should download a file and return a Blob
TypeError: this.client.request is not a function
❯ FileDownloader.download packages/effects/request/src/request-client/modules/downloader.ts:36:40
❯ packages/effects/request/src/request-client/modules/downloader.test.ts:28:41
🤖 Prompt for AI Agents
In packages/effects/request/src/request-client/modules/downloader.ts at line 36,
the direct call to this.client.request(url, finalConfig) causes a TypeError
because RequestClient may not implement a request method, breaking existing
clients and mocks. To fix this, implement a safe dispatch that first checks if
this.client.request exists and calls it if so; otherwise, for GET requests call
this.client.get(url, config), and for other HTTP methods call the corresponding
method-specific function on this.client. This preserves backward compatibility
without requiring immediate changes to RequestClient interface or its tests.
支持除GET以外的其他方法，由用户自己决定
Summary by CodeRabbit