perf: allow the downloader to support custom return types #5443
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/downloader.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 modifies the Changes
Suggested labels
Suggested reviewers
Possibly related PRs
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: 0
🧹 Nitpick comments (1)
packages/effects/request/src/request-client/modules/downloader.ts (1)
2-2: LGTM! Good abstraction of request types.The switch from direct Axios types to internal request types improves the module's flexibility and abstraction. This change aligns well with the goal of supporting custom return types.
This decoupling from Axios types will make it easier to:
- Support different HTTP clients in the future
- Customize request/response handling without being tied to Axios implementations
📜 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(2 hunks)
⏰ 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: post-update (windows-latest)
- GitHub Check: Lint (ubuntu-latest)
- GitHub Check: post-update (ubuntu-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
packages/effects/request/src/request-client/modules/downloader.ts (2)
13-13: LGTM! Method signature properly updated.The parameter type change to
RequestClientConfigmaintains consistency with the new type system while preserving the method's functionality.
15-15: Verify ResponseType compatibility in RequestClientConfig.While the type change is consistent, we should verify that 'blob' is a valid responseType in the new
RequestClientConfigtype definition.Let's check the type definition:
✅ Verification successful
Type change is valid and safe
The
RequestClientConfigtype extendsAxiosRequestConfigwhich supports 'blob' as a valid responseType. The type change is consistent with the codebase usage and Axios types.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for RequestClientConfig type definition to verify responseType options ast-grep --pattern 'type RequestClientConfig = { $$$ responseType$$$ $$$ }' # Also check for any type definitions that might extend or modify ResponseType rg -l "responseType.*=.*['\"]blob['\"]"Length of output: 129
Script:
#!/bin/bash # Search for RequestClientConfig references rg -l "RequestClientConfig" # Search for interface definition as an alternative ast-grep --pattern 'interface RequestClientConfig { $$$ }' # Search for type imports rg "import.*RequestClientConfig.*from" # Search for responseType in the effects/request directory fd . packages/effects/request -t f -x rg "responseType"Length of output: 724
Script:
#!/bin/bash # Show the content of types.ts focusing on RequestClientConfig cat packages/effects/request/src/request-client/types.ts # Backup: search for any type definitions related to responseType ast-grep --pattern 'type $_ResponseType = $$$'Length of output: 1749
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/effects/request/src/request-client/modules/downloader.ts (1)
Line range hint
11-27: Consider making the response type more generic.While the current implementation works well for blob downloads, consider making the response type configurable to support other types of downloads. This would align better with the PR's objective of supporting custom return types.
Example approach:
public async download<T = Blob>( url: string, config?: RequestClientConfig, ): Promise<RequestResponse<T>> { const finalConfig: RequestClientConfig = { ...config, responseReturn: 'raw', }; return this.client.get<RequestResponse<T>>(url, finalConfig); }
📜 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(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: post-update (windows-latest)
- GitHub Check: post-update (ubuntu-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: Test (windows-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: Lint (ubuntu-latest)
🔇 Additional comments (3)
packages/effects/request/src/request-client/modules/downloader.ts (3)
2-2: LGTM! Good abstraction of request types.The switch from Axios-specific types to custom request types improves modularity and allows for better flexibility in handling different HTTP clients.
13-14: LGTM! Type signature changes are consistent.The updated parameter and return types maintain type safety while enabling custom return type support as intended.
15-19: Verify the impact of the newresponseReturnconfiguration.The addition of
responseReturn: 'raw'might affect existing consumers of thedownloadmethod. Please ensure this change is documented and verify its compatibility with existing code.✅ Verification successful
The
responseReturn: 'raw'configuration is safe to useThe configuration property is already supported in the system's type definitions and interceptors. The change ensures consistent raw blob responses for downloads while maintaining compatibility with existing consumers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing usages of the download method to assess impact rg -l "download\(" | xargs rg -B 2 -A 2 "download\(" # Search for any existing uses of responseReturn configuration rg "responseReturn"Length of output: 4301
gee1k
changed the title
Description
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