Skip to content

QueryParameter returns URLSearchParams value #11

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

Conversation

iga-tak
Copy link
Contributor

@iga-tak iga-tak commented Apr 26, 2022

最終的なシリアライズを使う側でコントロールできるようにするために、QueryParameterはstringではなくURLSearchParamsを返すようにしました

if (params.explode) {
return Object.entries(params.value)
.map(([k, v]) => `${k}=${v}`)
.join(";");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CookieParameterはURLSearchParams使うの良くなさそうなので旧実装もってきて分離しました。
explode: true の場合の区切り文字は ; の方が適切な気がしたので変更しています。
Coreの方にあった方がよければそちらに移します。

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: formexplode: true でINPUTがobjectの場合以下の行に該当します。

image

RFC6570上で特にCookie Parameterについて言及を自分は見つけられませんでした。他にこの変更をサポートする仕様はありますか?

参考

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explode: true がそれぞれの変数を個別に展開して設定する、という意味であると解釈するとCookieの仕様 RFC6265 に沿ったフォーマットの方が適切であると考えます。
が、おっしゃる通りCookie Parameterにおいて style: form explode: true の仕様が明確になっておらず混乱の元となっています。

個人的にはこの仕様のCookie Parameterは使わないのでこの変更は不要ですが、上記OpenAPIのexample tableを守るなら & 区切りに戻します。

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

このPRはQueryParameterだけにフォーカスして、必要になったらCookie Parameterの方も調整しましょう 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

了解です

@Himenon Himenon self-requested a review April 26, 2022 11:04
src/Core.ts Outdated
@@ -36,75 +36,97 @@ export const generateFromSimple = (key: string | number, params: ParameterOfSimp
return undefined;
};

export const generateFormParamter = (key: string | number, params: ParameterOfForm): string => {
export const generateFormParamter = (key: string | number, params: ParameterOfForm): URLSearchParams | undefined => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

既存の関数で返り値が変更になるような破壊的変更は望みません。
この関数の内部ではstringを返すように変更をお願いします。

URLSearchParamsで返すようなAPIを既存の関数と独立してexportし、ライブラリの利用者がどちらも利用可能な状態にしてください。

他も同様です。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

把握です

}
return undefined;
};

const generateFormParamter = (key: string | number, params: ParameterOfForm): string => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

こちらの関数利用したい場合はテストされるようにexportし、テストを実施してください。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

把握です

@iga-tak iga-tak requested a review from Himenon April 29, 2022 03:54
@@ -90,7 +90,7 @@ describe("QueryParameter - style:spaceDelimited", () => {
style: "spaceDelimited",
explode: false,
});
expect(result1).toBe("blue%20black%20brown");
expect(result1).toBe("color=blue+black+brown");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ここはOpenAPI Formatterの仕様と異なるようです。

@@ -90,7 +90,7 @@ describe("QueryParameter - style:spaceDelimited", () => {
style: "spaceDelimited",
explode: false,
});
expect(result1).toBe("blue%20black%20brown");
expect(result1).toBe("color=blue+black+brown");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expect(result1).toBe("color=blue%20black%20brown");

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://qiita.com/masakielastic/items/61f5d9a215c62b55ccf2
このような違いがあるようです。このライブラリはOpenAPI準拠で表すので%20に寄せます。

@@ -102,7 +102,7 @@ describe("QueryParameter - style:spaceDelimited", () => {
style: "spaceDelimited",
explode: false,
});
expect(result1).toBe("R%20100%20G%20200%20B%20150");
expect(result1).toBe("color=R+100+G+200+B+150");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expect(result1).toBe("color=R%20100%20G%20200%20B%20150");

@Himenon
Copy link
Owner

Himenon commented Apr 29, 2022

Merged #12

@Himenon Himenon closed this Apr 29, 2022
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