-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Conversation
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.
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. 👀
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.
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
The |
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 |
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. |
This needs a rebase. |
eba16cb
to
5118eea
Compare
This needs a rebase. |
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>
af5d081
to
9c56db1
Compare
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>
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>
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:
@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).@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-fetchStatus and versioning classification: