-
Notifications
You must be signed in to change notification settings - Fork 58
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(rest): add wrapper client #3193
base: main
Are you sure you want to change the base?
Conversation
a700b27
to
e65d208
Compare
auth?: AuthOption; | ||
basicAuth?: BasicAuthOption; | ||
proxy?: Proxy; | ||
httpsAgent?: HttpsAgent; | ||
clientCertAuth?: ClientCertAuth; | ||
userAgent?: string; | ||
socketTimeout?: number; |
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.
この辺りは OR | AND の関係性のオプションがあれば、それを型に反映したいです。
例えば httpsAgent
はいくつかのHTTP or TCPレイヤーの設定と排他になっているので、それを反映してもらうと良さそうです。
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.
@tasshi-me
書き方が適切か見ていただきたいのですが、proxy, httpsAgent, clientCertAuthを1つだけしか指定できないようにしました。
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.
あーなるほど、
単純に↓のようなunion typeにしても余剰プロパティチェックが効かないのでneverを入れてるんですね。
| {
proxy?: Proxy;
}
| {
httpsAgent?: HttpsAgent;
}
| {
certAuth?: ClientCertAuth;
}
型補完が分かりにくくなるのでやはりdiscriminated unionかなーとも思いましたが、利用者が直接使うオプションなのでkindsを指定させるほうがややこしそう。
これで良さそうです!
export type ApiTokenAuth = { | ||
apiToken: string | string[]; | ||
}; | ||
|
||
export type PasswordAuth = { | ||
username: string; | ||
password: string; | ||
}; | ||
|
||
export type SessionAuth = {}; | ||
|
||
export type OAuthTokenAuth = { | ||
oAuthToken: string; | ||
}; |
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.
ここは discriminated union でもいいかもしれないですが、どっちでも良さそう。
(プロパティ名で判定できているし、認証の必要情報が大きく変わることはなさそう)
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.
こちらはdiscriminated unionに変更しました。
48485eb
to
eb187e7
Compare
eb187e7
to
a721a3c
Compare
export const createClient = (clientOptions: KintoneClientOptions) => { | ||
return _createClient<paths>(clientOptions); | ||
}; |
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.
pathsが巨大すぎるので、_createClientをラップせずpathsをジェネリクスで渡さず直接コーディングすると、型が壊れてしまう。
} & ( | ||
| { | ||
proxy?: Proxy; | ||
httpsAgent?: never; | ||
certAuth?: never; | ||
} | ||
| { | ||
proxy?: never; | ||
httpsAgent?: HttpsAgent; | ||
certAuth?: never; | ||
} | ||
| { | ||
proxy?: never; | ||
httpsAgent?: never; | ||
certAuth?: ClientCertAuth; | ||
} |
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.
never型を指定することでどれか一つしかとれないようにした
[key: string]: unknown; | ||
}; | ||
|
||
type KintoneMethodType = Extract<HttpMethod, "get" | "post" | "put" | "delete">; |
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.
Extractを使えばHttpMethodの一部であることを明示できる、多分
Why
We want to improve the development experience using rest.js.
What
createClient()
to reduce the code and improve the developer experience.createClient()
method similar to those of rest-api-client.api()
to the client, whose interface is similar tokintone.api()
of JavaScript APIHow to test
Checklist
pnpm lint
andpnpm test
on the root directory.