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.

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(Client): add apiResponse and apiRequest events #6739

Merged
merged 4 commits into from Oct 9, 2021

Conversation

ckohen
Copy link
Member

@ckohen ckohen commented Oct 2, 2021

Please describe the changes this PR makes and why it should be merged:

Adds an event emitted on every response received from the API, closes: #6707
This is quite useful for debugging purposes, especially when trying to figure out if something may be a Discord bug and not on the library side.

Some things to consider:

  • Name of the event, I feel like this is accurate now, but naming things is hard
  • In order to accomplish this, I've added docs / types to APIRequest, which makes changing it potentially breaking (e.g. if we wanted to fully replace rest with @discordjs/rest). To avoid this I could pull out specific properties from the request and only emit those, bonus to this being less chance of mutating (only matters when the request needs to be retried).
  • In order to type the response, I had to add @types/node-fetch (since we don't target DOM), which does not have a version matching the version in the library since v3 has built in types. This could easily be solved by refactor: node-fetch & @discordjs/formdata -> undici #6586 or updating node-fetch

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating
  • This PR changes the library's interface (methods or parameters added)

typings/index.d.ts Outdated Show resolved Hide resolved
src/rest/APIRequest.js Outdated Show resolved Hide resolved
src/rest/RequestHandler.js Outdated Show resolved Hide resolved
src/rest/RequestHandler.js Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
@iCrawl iCrawl added this to the Version 13.2 milestone Oct 2, 2021
Copy link
Member

@SpaceEEC SpaceEEC left a comment

Choose a reason for hiding this comment

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

I think the event name is fine.

I'd be in favor of extracting relevant properties of APIRequest into an interface instead of documenting and emitting it.

This also does not seem to address the request for an apiRequest (when a request is being made) event, which was part of the linked issue. 👀

Copy link
Member

@vladfrangu vladfrangu left a comment

Choose a reason for hiding this comment

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

Main issue I see is below. Also, I agree with @SpaceEEC, we shouldn't document the full internal class we have but just an "object" of it (honestly you might be able to just do { ...request } as well), because users shouldn't call make() themselves

src/rest/RequestHandler.js Outdated Show resolved Hide resolved
@smol-skyz
Copy link

This also does not seem to address the request for an apiRequest (when a request is being made) event, which was part of the linked issue. 👀

The apiResponse event is what I was looking for, though it would be ideal to also have an apiRequest event too since some use cases might not require the response and I'm not sure what would happen if the request actually fails (if the event would get emitted or not).

@KhafraDev
Copy link
Contributor

which does not have a version matching the version in the library since v3 has built in types. This could easily be solved by refactor: node-fetch & @discordjs/formdata -> undici #6586 or updating node-fetch

Discord.js cannot upgrade to node-fetch v3 due to it being ESM-only whilst this project is CJS.

@ckohen
Copy link
Member Author

ckohen commented Oct 4, 2021

Discord.js cannot upgrade to node-fetch v3 due to it being ESM-only whilst this project is CJS.

I think we'd be able to async import it. Regardless, that's the harder of the options at this point

@ckohen
Copy link
Member Author

ckohen commented Oct 4, 2021

At what point would you expect the apiRequest event to be fired? I'm going to put it right before the request for now, but there could be an argument for emitting before waiting out ratelimits.

@ckohen ckohen changed the title feat(Client): add apiResponse event for debugging feat(Client): add apiResponse and apiRequest events Oct 4, 2021
@iCrawl iCrawl modified the milestones: Version 13.2, Version 13.3 Oct 4, 2021
@iCrawl
Copy link
Member

iCrawl commented Oct 5, 2021

This needs a rebase.

@iCrawl
Copy link
Member

iCrawl commented Oct 8, 2021

This needs a rebase.

ckohen and others added 3 commits October 8, 2021 15:58
Co-Authored-By: Shubham Parihar <shubhamparihar391@gmail.com>
Co-Authored-By: Rodry <38259440+ImRodry@users.noreply.github.com>
Co-Authored-By: Vlad Frangu <kingdgrizzle@gmail.com>
Co-Authored-By: SpaceEEC <24881032+SpaceEEC@users.noreply.github.com>
@iCrawl iCrawl merged commit 26f927b into discordjs:main Oct 9, 2021
@ckohen ckohen deleted the apiResponse-event branch October 9, 2021 22:20
ckohen added a commit to ckohen/discord.js-modules that referenced this pull request Dec 2, 2021
Ported from discordjs/discord.js#6739

Co-authored-by: Shubham Parihar <shubhamparihar391@gmail.com>
Co-authored-by: Rodry <38259440+ImRodry@users.noreply.github.com>
Co-authored-by: Vlad Frangu <kingdgrizzle@gmail.com>
Co-authored-by: SpaceEEC <24881032+SpaceEEC@users.noreply.github.com>
ckohen added a commit to ckohen/discord.js-modules that referenced this pull request Dec 2, 2021
Ported from discordjs/discord.js#6739

Co-authored-by: Shubham Parihar <shubhamparihar391@gmail.com>
Co-authored-by: Rodry <38259440+ImRodry@users.noreply.github.com>
Co-authored-by: Vlad Frangu <kingdgrizzle@gmail.com>
Co-authored-by: SpaceEEC <24881032+SpaceEEC@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an event for getting http requests and responses
10 participants