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

Jump to bottom

feat(rest): use undici #7747

Merged
merged 58 commits into from
May 12, 2022
Merged

feat(rest): use undici #7747

merged 58 commits into from
May 12, 2022

Conversation

KhafraDev
Copy link
Contributor

@KhafraDev KhafraDev commented Apr 5, 2022 •
edited
Loading

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

Transfers from node-fetch and form-data to undici.

WIP:

  • Finish re-writing tests and remove nock.
  • Report a bunch of mock bugs I've found with undici.
  • Use undici.request when a FormData body isn't being sent because it's way faster.
  • Replace postFile tests

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes (WIP)
  • I know how to update typings and have done so, or typings don't need updating
  • This PR includes breaking changes (methods removed or renamed, parameters moved or removed) (just to be safe)

@ckohen
Copy link
Member

ckohen commented Apr 5, 2022

oh good, I was literally gonna start working on this in like 20 minutes, I can't write directly from here so I'll PR tests to your fork

@Rexogamer Rexogamer mentioned this pull request Apr 5, 2022
Merged
@KhafraDev KhafraDev marked this pull request as ready for review April 5, 2022 19:40
@KhafraDev

This comment was marked as outdated.

Sign in to view
vladfrangu
vladfrangu requested changes Apr 5, 2022
View reviewed changes
packages/rest/__tests__/RequestHandler.test.ts Outdated Show resolved Hide resolved
packages/rest/__tests__/RequestManager.test.ts Outdated Show resolved Hide resolved
packages/rest/__tests__/REST.test.ts Outdated Show resolved Hide resolved
packages/rest/src/lib/RequestManager.ts Outdated Show resolved Hide resolved
packages/rest/src/lib/RequestManager.ts Outdated Show resolved Hide resolved
packages/rest/src/lib/RequestManager.ts Show resolved Hide resolved
@KhafraDev KhafraDev force-pushed the undici-finally-wow branch from d9dd20e to 020a380 Compare April 5, 2022 23:01
@KhafraDev KhafraDev requested a review from vladfrangu April 6, 2022 00:14
ckohen
ckohen requested changes Apr 6, 2022
View reviewed changes
Copy link
Member

@ckohen ckohen left a comment

Choose a reason for hiding this comment

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

Looks like you didn't need me to write tests after all 😌

Just a few nits, also looks like you should regen your lockfile, it still has the collection version change in it.

packages/rest/__tests__/RequestHandler.test.ts Outdated Show resolved Hide resolved
packages/rest/src/lib/RequestManager.ts Outdated Show resolved Hide resolved
packages/rest/src/lib/RequestManager.ts Outdated Show resolved Hide resolved
packages/rest/src/lib/RequestManager.ts Outdated Show resolved Hide resolved
packages/rest/src/lib/handlers/SequentialHandler.ts Outdated Show resolved Hide resolved
packages/rest/src/lib/handlers/SequentialHandler.ts Show resolved Hide resolved
@KhafraDev
Copy link
Contributor Author

This PR is ready. Only thing missing is a real replacement to the response event but one of the undici maintainers didn't like my proposed solution to sending a consumed body (see: nodejs/undici#1369). The event itself was re-added in 483719a.

However, all the bugs I found were fixed and all the tests are now running. Please merge so I don't have to maintain this pr permanently 🙏.

@KhafraDev KhafraDev requested a review from vladfrangu May 10, 2022 02:39
kyranet
kyranet requested changes May 12, 2022
View reviewed changes
packages/discord.js/package.json Outdated Show resolved Hide resolved
packages/rest/__tests__/REST.test.ts Outdated Show resolved Hide resolved
packages/rest/package.json Outdated Show resolved Hide resolved
@KhafraDev KhafraDev requested a review from kyranet May 12, 2022 20:01
@iCrawl iCrawl merged commit d1ec8c3 into discordjs:main May 12, 2022
@KhafraDev
Copy link
Contributor Author

Oh

@KhafraDev KhafraDev deleted the undici-finally-wow branch May 12, 2022 20:53
@didinele didinele mentioned this pull request May 15, 2022
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@almeidx almeidx almeidx left review comments

@iCrawl iCrawl iCrawl approved these changes

@ckohen ckohen ckohen requested changes

@SpaceEEC SpaceEEC SpaceEEC approved these changes

@vladfrangu vladfrangu Awaiting requested review from vladfrangu

@kyranet kyranet Awaiting requested review from kyranet

Assignees
No one assigned
Labels
packages:discord.js packages:rest refactor semver:major
Projects
discord.js
Archived in project
Milestone
discord.js v14
Development

Successfully merging this pull request may close these issues.
7 participants
@KhafraDev @ckohen @vladfrangu @iCrawl @kyranet @SpaceEEC @almeidx