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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

rest: prefer arrayBuffer over buffer #7318

Merged
merged 2 commits into from Feb 16, 2022
Merged

rest: prefer arrayBuffer over buffer #7318

merged 2 commits into from Feb 16, 2022

Conversation

KhafraDev
Copy link
Contributor

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

  • Response.buffer is not a spec compliant method.
  • Buffer only exists in node and it's just a wrapper for Uin8Arrays anyways.
  • It's getting removed in v4, if its decided to stay with node-fetch and move towards ESM.

I have no idea if types need updating, sorry 馃槙

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes
  • This PR changes the library's interface (methods or parameters added)
  • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)

@KhafraDev
Copy link
Contributor Author

KhafraDev commented Jan 22, 2022

Also pinging @ckohen since they're pretty involved in rest

@ckohen
Copy link
Member

ckohen commented Jan 22, 2022

At first glance, I'm inclined to not make this switch until we utilize undici because of the way node-fetch handles consuming the body. Internally, node-fetch seems to much prefer using buffer() since that is what their consume function converts to natively (Their consume body function probably is why buffer() exists at all). While small, there is a slight performance hit from converting that buffer, not sure if that's going to end up mattering much though.

I guess its a matter of if we want to do part of the work now rather than later (also makes undici a semver:minor / patch PR I think?) which is up to the maintainers.

To note: should be obvious, but this will need changes in the main lib in #7298

@KhafraDev
Copy link
Contributor Author

KhafraDev commented Jan 22, 2022

I doubt that this change will have any meaningful performance impact, as node-fetch's arrayBuffer method is simply just using buffer.slice after calling consumeBody.

The only difference is that users will have to deal with ArrayBuffers instead of Buffers on api routes that don't have an application/json Content-Type header. Even then, converting from ArrayBuffer to Buffers (Uint8Arrays) will not be slowing down user applications 馃槃.

@ckohen
Copy link
Member

ckohen commented Jan 22, 2022

Yeah, slight could definitely be considered an exaggeration for the performance implications lol, especially considering the slim amount of endpoints that don't return JSON to begin with

@iCrawl iCrawl added this to the rest v1 milestone Jan 26, 2022
@jimmywarting
Copy link

converting it to a node buffer have not so much cost to it i think,
quite simple to do:

const buffer = await res.arrayBuffer().then(Buffer.from)

But ultimately i think buffer should be discouraged for better cross platform functionality
https://github.com/cross-js/cross-js#dont-use-buffer

@iCrawl
Copy link
Member

iCrawl commented Feb 15, 2022

Can you rebase this PR? @KhafraDev

@KhafraDev
Copy link
Contributor Author

@iCrawl rebased 馃憤

@iCrawl iCrawl merged commit 868e2f3 into discordjs:main Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

6 participants