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.

Sign up for GitHub

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

chore: enhance client-related typings #1227

Merged
merged 3 commits into from
Jan 12, 2021
Merged

chore: enhance client-related typings #1227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0e8d261
Enhance client-related typings
kyle221b Dec 5, 2020
1427e5e
Style fix
kyle221b Dec 17, 2020
8514165
Merge branch 'main' into typing-enhancements
JenniferMah Jan 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/client/src/client.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this change might break folks: #986

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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) {
...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@kyle221b kyle221b Dec 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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) {
...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

export = client

export {Client};
Copy link
Contributor

Choose a reason for hiding this comment

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

We would need to revert this change also if we move back to declare const client: Client & { Client: typeof Client };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

left a comment above about why I did this. let me know what you think

3 changes: 3 additions & 0 deletions packages/helpers/classes/request.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import * as https from 'https';

type HttpMethod = 'get'|'GET'|'post'|'POST'|'put'|'PUT'|'patch'|'PATCH'|'delete'|'DELETE';

export default interface RequestOptions<TData = any, TParams = object> {
Expand All @@ -7,4 +9,5 @@ export default interface RequestOptions<TData = any, TParams = object> {
qs?: TParams;
body?: TData;
headers?: object;
httpsAgent?: https.Agent;
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this data is stored in headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
6 changes: 6 additions & 0 deletions packages/mail/src/mail.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import {Client} from "@sendgrid/client";
import {ClientResponse} from "@sendgrid/client/src/response";
import {ResponseError} from "@sendgrid/helpers/classes";
import {MailDataRequired} from "@sendgrid/helpers/classes/mail";
Expand All @@ -8,6 +9,11 @@ declare class MailService {
*/
setApiKey(apiKey: string): void;

/**
* Client to use for invoking underlying API
*/
setClient(client: Client): void;

/**
* Twilio Email Auth passthrough for convenience.
*/
Expand Down
4 changes: 3 additions & 1 deletion test/typescript/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import Client = require("@sendgrid/client");
import * as https from 'https';

// Test setApiKey() method
Client.setApiKey("MY_SENDGRID_API_KEY");
Expand All @@ -11,7 +12,8 @@ Client.setDefaultHeader({
// Test setDefaultRequest() method
Client.setDefaultRequest({
url: "/test",
}).setDefaultRequest("method", "POST");
}).setDefaultRequest("method", "POST")
.setDefaultRequest('httpsAgent', new https.Agent())

// Test createHeaders() method
Client.createHeaders({
Expand Down
4 changes: 4 additions & 0 deletions test/typescript/mail.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
import { Client } from "@sendgrid/client";
import sgMail = require("@sendgrid/mail");

// Test setApiKey() method
sgMail.setApiKey("MY_SENDGRID_API_KEY");

// Test setClient() method
sgMail.setClient(new Client());

// Test setSubstitutionWrappers() method
sgMail.setSubstitutionWrappers("{{", "}}")

Expand Down