-
Notifications
You must be signed in to change notification settings - Fork 7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(@vben/request): export basic HttpResponse generic #4697
Conversation
|
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/effects/request/src/request-client/modules/downloader.ts (1)
Line range hint
22-26
: LGTM:get
method call updated correctly. Consider a minor improvement.The update of the type parameter in the
get
method call fromAxiosResponse<Blob>
toRequestResponse<Blob>
is consistent with the earlier changes and ensures type consistency throughout the method.For improved code consistency, consider using the
Blob
type directly in thefinalConfig
object:const finalConfig: AxiosRequestConfig = { ...config, - responseType: 'blob', + responseType: 'blob' as const, };This change makes the type more explicit and consistent with the usage in the
get
method call.packages/effects/request/src/request-client/types.ts (1)
7-7
: LGTM! Consider adding a brief comment for clarity.The addition of the
RequestResponse
type alias is a good way to make the code more domain-specific. It provides a clear, project-specific name for the Axios response type while maintaining flexibility with the generic parameter.Consider adding a brief comment to explain the purpose of this type alias, for example:
/** Alias for AxiosResponse to provide a more domain-specific type name */ type RequestResponse<T = any> = AxiosResponse<T>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/effects/request/src/request-client/modules/downloader.ts (2 hunks)
- packages/effects/request/src/request-client/types.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
packages/effects/request/src/request-client/modules/downloader.ts (3)
1-5
: LGTM: Import changes are consistent with type updates.The addition of the
RequestResponse
import and the removal of theAxiosResponse
import (as mentioned in the AI summary) align well with the changes in the method signature. This change supports the transition from Axios-specific types to custom types while maintaining the necessaryAxiosRequestConfig
import.
13-16
: LGTM: Method signature updated correctly.The return type change from
Promise<AxiosResponse<Blob>>
toPromise<RequestResponse<Blob>>
is consistent with the transition to custom types. It maintains type safety for blob responses while decoupling from Axios-specific types. The unchanged method parameters correctly retain the use ofAxiosRequestConfig
.
Line range hint
1-33
: Overall assessment: Changes are well-implemented and consistent.The modifications in this file successfully transition from Axios-specific types to custom types while maintaining type safety and consistency. The changes align well with the PR objectives of exporting a basic
HttpResponse
generic. No breaking changes or potential issues were identified.To ensure the
RequestResponse
type is correctly defined and exported, please run the following verification script:✅ Verification successful
Verified:
RequestResponse
type is correctly defined and utilized.The
RequestResponse
type is properly defined and exported inpackages/effects/request/src/request-client/types.ts
. Its usage indownloader.ts
is consistent and maintains type safety. No issues were identified.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the RequestResponse type definition and export # Test: Check if RequestResponse is defined and exported in types.ts rg -A 5 'export.*RequestResponse' packages/effects/request/src/request-client/types.ts # Test: Verify usage of RequestResponse in other files rg 'RequestResponse' packages/effects/request/src/request-client/Length of output: 681
packages/effects/request/src/request-client/types.ts (2)
51-51
: LGTM! Export is correctly placed and maintains alphabetical order.The
RequestResponse
type is appropriately exported along with other related types. This makes it available for use throughout the project, which aligns with the intention of the change. The alphabetical ordering of exports is maintained, which is good for readability and consistency.
Line range hint
1-53
: Overall, the changes enhance type safety and readability. Consider extending this pattern to other Axios types.The addition of the
RequestResponse
type alias is a positive change that makes the code more domain-specific and potentially easier to maintain. It's consistent with existing types and naming conventions in the file.As a future improvement, consider applying this pattern to other Axios types used in the project. For example, you might want to create aliases for
AxiosRequestConfig
orAxiosError
. This would further enhance the domain-specific nature of your types and make it easier to adapt if you ever need to switch underlying HTTP client libraries.To ensure consistency across the codebase, let's check for other usages of
AxiosResponse
:If there are other occurrences, consider updating them to use
RequestResponse
for consistency.
改动解释:例如我刚刚的一个业务需求是需要做文件导出下载功能,使用了 RequestClient 提供的 download 函数请求 Blob 数据。 然后我封装了一个 useDownload Hook 来进行创建 a 标签执行下载,我需要拿到 response 里的 content-disposition header 数据来获取文件名等信息,这时候原始的 AxiosResponse 类型没有导出导致我无法在 hook 里进行类型标注 |
下方代码两个 FIXME 处 import { message, notification } from 'ant-design-vue';
// FIXME: 等待类型导出
// import type { RequestResponse } from '@vben/request';
interface NavigatorWithMsSaveOrOpenBlob extends Navigator {
msSaveOrOpenBlob: (blob: Blob, fileName: string) => void;
}
export interface DownloadOptions {
isNotify?: boolean;
fileName?: string;
fileType?: string;
}
const defaultOptions: DownloadOptions = {
isNotify: false,
fileType: '.xlsx',
};
/**
* @description 接收数据流生成 blob,创建链接,下载文件
* @param {Function} api 导出表格的api方法 (必传)
* @param {object} options 配置项
* @param {boolean} options.isNotify 是否提示下载过程中的提示,默认为 false
* @param {string} options.fileName 文件名,默认为服务器返回或当前时间戳
* @param {string} options.fileType 文件类型,默认为服务器返回或 .xlsx
*/
export async function useDownload(
api: () => Promise<any>,
// FIXME: 等待类型导出
// api: () => Promise<RequestResponse<Blob>>,
options: DownloadOptions = defaultOptions,
) {
const loadingClose = message.loading({
content: '正在导出数据,请稍后...',
duration: 0,
});
try {
const res = await api();
const fileType = options?.fileType || '.xlsx';
const tempName = options?.fileName
? options.fileName + fileType
: // eslint-disable-next-line unicorn/no-nested-ternary
res.headers['content-disposition']
? decodeURI(
res.headers['content-disposition'].split(';')[1].split('=')[1],
).replaceAll('"', '')
: Date.now() + fileType;
loadingClose();
if (options?.isNotify) {
notification.warning({
message: '温馨提示',
description: '如果数据庞大会导致下载缓慢哦,请您耐心等待!',
});
}
if (
res.status !== 200 ||
res.data === null ||
!(res.data instanceof Blob)
) {
message.error('导出失败,请稍后再试!');
return;
}
const blob = new Blob([res.data]);
// 兼容 edge 不支持 createObjectURL 方法
if (
'msSaveOrOpenBlob' in
(navigator as unknown as NavigatorWithMsSaveOrOpenBlob)
) {
(
window.navigator as unknown as NavigatorWithMsSaveOrOpenBlob
).msSaveOrOpenBlob(blob, tempName + fileType);
}
const blobUrl = window.URL.createObjectURL(blob);
const exportFile = document.createElement('a');
exportFile.style.display = 'none';
exportFile.download = tempName;
exportFile.href = blobUrl;
document.body.append(exportFile);
exportFile.click();
// 去除下载对 url 的影响
exportFile.remove();
window.URL.revokeObjectURL(blobUrl);
} catch {
// console.log(error)
loadingClose();
}
} |
Description
Type of change
Please delete options that are not relevant.
pnpm-lock.yaml
unless you introduce a new test example.Checklist
pnpm run docs:dev
command.pnpm test
.feat:
,fix:
,perf:
,docs:
, orchore:
.Summary by CodeRabbit
New Features
RequestResponse
for improved type representation of responses.Bug Fixes
download
method to return a more specific response type, enhancing reliability in handling downloads.