chore: enhance client-related typings #1227
Conversation
The MailService has a method to set a client, but it isn't exposed in typings. Adding this so there is an easier way to customize underlying agents used (for keep-alive and other connection-pooling settings) without having compilation problems (or subverting the type system by just using 'any')
thinkingserious
added
the
status: code review request
| @@ -46,5 +46,8 @@ declare class Client { | |||
| request(data: ClientRequest, cb?: (err: ResponseError, response: [ClientResponse, any]) => void): Promise<[ClientResponse, any]>; | |||
| } | |||
|
|
|||
| declare const client: Client & { Client: typeof Client }; | |||
| declare const client: Client; | |||
| // @ts-ignore | |||
There was a problem hiding this comment.
This matches style of mail.d.ts file. Part that I wanted to change here mainly was the { Client: typeof Client } piece. I get the feeling that the goal there was to expose the class as a type, but we do typeof on the class itself, instead of an instance of it, so it ends up being a type with just a prototype field. Let me know if I'm missing something here.
There was a problem hiding this comment.
I believe this change might break folks: #986
There was a problem hiding this comment.
yeah the goal here was to export Client as not just a value with type typeof Client, but to export the class itself, which could be used as a value or a type. im pretty sure this will only enhance the existing functionality. currently, users could do:
const a = new Client()
but the only way they could pass the actual type around is by doing a default import of the singleton client and then doing a typeof on it.
am i missing something here?
There was a problem hiding this comment.
I don't quite understand why this method is better than what we have. Perhaps a good way to illustrate is to add a sample quick start here.
There was a problem hiding this comment.
To give a quick code example:
import { Client } from '@sendgrid/client';
// This is possible in both versions
const client = new Client();
// This (I think) is only possible in the new version since Client is only a value in the old version and not a type
function somethingThatTakesInAClient(client: Client) {
...
}
There was a problem hiding this comment.
while it isn't necessary, you do it for most of your other types that I've seen, and if you are going to export functionality, it seems to me it would also make sense to export the type
There was a problem hiding this comment.
to further illustrate, in the existing code, we could accomplish the same by doing:
// Note we are importing the singleton client exported by the lib as well as the class itself (as a value)
import client, { Client } from '@sendgrid/client';
// We can do a typeof on the singleton instance. This is equivalent to what is in the last example. For this one you need // to import a singleton instance of the client just to get the type
function somethingThatTakesInAClient(arg: typeof client) {
...
}
// This compiles, but the behavior isn't what you expect. Since we are doing typeof on the class, the only members are // things on the function prototype, so we can't access methods like 'setApiKey' for example
function anotherThingThatLooksLikeItTakesInAClientButDoesntReally(arg: typeof Client) {
...
}
There was a problem hiding this comment.
Here's an example where you use the proposed pattern: https://github.com/sendgrid/sendgrid-nodejs/blob/main/packages/mail/src/mail.d.ts#L39-L42.
|
@thinkingserious any chance you or someone from your team could take a look at this? |
thinkingserious
added
the
type: community enhancement
| @@ -46,5 +46,8 @@ declare class Client { | |||
| request(data: ClientRequest, cb?: (err: ResponseError, response: [ClientResponse, any]) => void): Promise<[ClientResponse, any]>; | |||
| } | |||
|
|
|||
| declare const client: Client & { Client: typeof Client }; | |||
| declare const client: Client; | |||
| // @ts-ignore | |||
There was a problem hiding this comment.
I believe this change might break folks: #986
| export = client | ||
|
|
||
| export {Client}; |
There was a problem hiding this comment.
We would need to revert this change also if we move back to declare const client: Client & { Client: typeof Client };
There was a problem hiding this comment.
left a comment above about why I did this. let me know what you think
| @@ -7,4 +9,5 @@ export default interface RequestOptions<TData = any, TParams = object> { | |||
| qs?: TParams; | |||
| body?: TData; | |||
| headers?: object; | |||
| httpsAgent?: https.Agent; | |||
There was a problem hiding this comment.
I believe this data is stored in headers.
There was a problem hiding this comment.
this is a top-level field on an axios request. i don't think we'd want to pass it in headers b/c it won't actually get sent across the wire, it's just connection settings essentially.
if we were to add it to headers, then there would need to be a code change in the JS file to filter it out and pass it as a top-level field in the axios request
|
Your reasoning on this thread makes sense to me. However, I'm still concerned about the points made in this PR. @danhlee329, @dfcowell, @manusis, @mgummelt -- do you have any thoughts on this issue? Thanks! With best regards, Elmer |
|
Anybody have thoughts on this? |
Fixes
The MailService has a method to set a client, but it isn't exposed in typings. Adding this so there is an easier way to customize underlying agents used (for keep-alive and other connection-pooling settings) without having compilation problems (or subverting the type system by just using 'any')
Checklist
If you have questions, please file a support ticket, or create a GitHub Issue in this repository.