Skip to content
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: putBucketCORS support responseVary config #1143

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Dennis273
Copy link

@Dennis273 Dennis273 commented Jul 22, 2022

Support update responseVary in CORS configuration.

Related documentation

TODOS:

  • add tests
  • update README.md

@Dennis273
Copy link
Author

Dennis273 commented Jul 22, 2022

@PeterRao Hi,打扰下。想在 putBucketCORS 中新增 responseVary 字段。当前参数设计导致不太好增加字段,打算将参数2调整为 Array<CORSRules> | { rules: Array<CORSRules>, responseVary?: boolean }。想看下合不合适🤔

@PeterRao PeterRao requested a review from taotao7 July 26, 2022 02:58
@taotao7
Copy link
Contributor

taotao7 commented Jul 26, 2022

Related documentation

稍等我等下本地测试下

@taotao7
Copy link
Contributor

taotao7 commented Jul 26, 2022

@PeterRao Hi,打扰下。想在 putBucketCORS 中新增 responseVary 字段。当前参数设计导致不太好增加字段,打算将参数2调整为 Array<CORSRules> | { rules: Array<CORSRules>, responseVary?: boolean }。想看下合不合适🤔

感觉是不是加在options改动少点也不破坏现有逻辑

@Dennis273
Copy link
Author

@PeterRao Hi,打扰下。想在 putBucketCORS 中新增 responseVary 字段。当前参数设计导致不太好增加字段,打算将参数2调整为 Array<CORSRules> | { rules: Array<CORSRules>, responseVary?: boolean }。想看下合不合适🤔

感觉是不是加在options改动少点也不破坏现有逻辑

@taotao7
一开始是考虑加在 options 里的,不过看了下这几个 API 中 options 都是设计为请求参数(例如 timeout)(ts 定义中这个参数的咧类型也是 OSS.RequestOptions)。直接加在 options 会导致其“语义”上与其他 API 不一致。

Array<CORSRules> | { rules: Array<CORSRules>, responseVary?: boolean } 的设计保持了对现有调用的兼容,同时 object 的设计更贴近接口文档中的 CORSConfiguration,未来要改动也比较简单。(代价就是实现上需要去兼容 Array | object)。

考虑两者:

  • 加在 options 中:
    • 优点:实现简单;
    • 缺点:影响了 options 语义,与其他 API 不一致。
  • 调整参数2:
    • 优点:参数设计与接口设计更接近,语义上更合理;
    • 缺点:实现上需要对旧调用进行兼容,处理 Array | object

我个人更倾向于后者(即调整参数2),不过还是看你们的决定🤔

@taotao7
Copy link
Contributor

taotao7 commented Jul 26, 2022

@PeterRao 琦飞老师你怎么看呢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants