Top-gg-Community / node-sdk

An official module for interacting with the Top.gg API
https://topgg.js.org/
133 stars 42 forks source link

src: replace legacy module #60

Closed VoltrexKeyva closed 3 years ago

VoltrexKeyva commented 3 years ago

The querystring module is legacy now, it is recommended to use the built-in URLSearchParams constructor.

Xetera commented 3 years ago

Can we get rid of all references to querystring and remove it as a dependency?

jpbberry commented 3 years ago

Can we get rid of all references to querystring and remove it as a dependency?

^, replace in Webhook implementation

Also I just approved your build run and it failed.

VoltrexKeyva commented 3 years ago

The querystring module is built-in but legacy as we don't have it as a dependency, also yes we can remove it and replace it with the recommended methods instead.

Xetera commented 3 years ago

oh I thought the built in module was query-string lol. I love the javascript dependency ecosystem

jpbberry commented 3 years ago

Just replace

- private async _request (method: string, path: string, body?: object): Promise<any> {
+ private async _request (method: string, path: string, body?: {}): Promise<any> {
jpbberry commented 3 years ago

Remove the ts-ignore

Xetera commented 3 years ago

Why are we replacing object with an empty object literal instead of Record<string, string>

jpbberry commented 3 years ago

Why are we replacing object with an empty object literal instead of Record<string, string>

Numbers are passed that aren't strings. So a type error will occur because Record<string, string|number> wouldn't match USPs Record<string, string>. It could work with Record<string, any> now that I think about it.

VoltrexKeyva commented 3 years ago

We also have this line in /src/structs/Webhook.ts using querystring.parse(): https://github.com/top-gg/node-sdk/blob/cb60671c69a53728dc85d9b1483e47b80d805873/src/structs/Webhook.ts#L50 Is this also formatting query parameters from a key value object to a string? Or what is this doing here?

jpbberry commented 3 years ago

That's converting a string into it's key value object.

jpbberry commented 3 years ago

@Xetera should we keep {}, Record<string, any>, or maybe cast body in the URLSearchParams constructor?

VoltrexKeyva commented 3 years ago

cc @Xetera

Xetera commented 3 years ago

I'd rather keep it as Record<string, any>, {} doesn't make much sense here