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

refactor(rest): add web compatibility #9682

Merged
merged 19 commits into from
Jul 9, 2023
Merged

refactor(rest): add web compatibility #9682

merged 19 commits into from
Jul 9, 2023

Conversation

suneettipirneni
Copy link
Member

@suneettipirneni suneettipirneni commented Jul 3, 2023 •
edited
Loading

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

Allow /rest to run in edge/web environments.

  • node: prefixes for value imports have been removed in favor of global objects.
  • ResponseLike now has an additional property statusText as STATUS_CODES is only in node and fetch's statusText seems to provide equivalent functionality
  • Replace EventEmitter with AsyncEventEmitter.
  • file-type has been removed due to dependence on node and magic-bytes.js is it's replacement
  • We now build two entrypoints for both web and node environments. When importing the package in a node env, it will default to using the undici implementation. When imported in an edge/web env it will default to global 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)
  • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)

@suneettipirneni suneettipirneni self-assigned this Jul 3, 2023
@vercel
Copy link

vercel bot commented Jul 3, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
discord-js ⬜️ Ignored (Inspect) Jul 7, 2023 11:48pm
discord-js-guide ⬜️ Ignored (Inspect) Jul 7, 2023 11:48pm

@Jiralite Jiralite added this to the rest 2.0.0 milestone Jul 3, 2023
vladfrangu
vladfrangu requested changes Jul 3, 2023
View reviewed changes
package.json Outdated Show resolved Hide resolved
packages/rest/package.json Outdated Show resolved Hide resolved
packages/rest/src/lib/RequestManager.ts Outdated Show resolved Hide resolved
packages/rest/src/lib/utils/constants.ts Outdated Show resolved Hide resolved
packages/rest/src/lib/utils/utils.ts Outdated Show resolved Hide resolved
ckohen
ckohen requested changes Jul 3, 2023
View reviewed changes
packages/rest/.eslintrc.json Show resolved Hide resolved
packages/rest/src/lib/REST.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/Shared.ts Outdated Show resolved Hide resolved
packages/rest/src/lib/utils/constants.ts Outdated Show resolved Hide resolved
packages/rest/tsup.config.ts Outdated Show resolved Hide resolved
@suneettipirneni
refactor(rest): add web compatibility
38d90d2 
This PR makes the following changes:

- `node:` prefixes for *value* imports have been removed in favor
of global objects.
- `ResponseLike` not has an additional property `statusText` as
`STATUS_CODES` is only in node and `fetch`'s statusText seems to provide
equivalent functionality
- `file-type` has been removed due to dependence on node and `is-apng`
is it's replacement
- Various fixes have been made to how the undici strategy is imported.
`undiciRequest` was being bundled even if `fetch` was specified as the
`makeRequest` option, despite it being a dynamic import. This PR fixes
that issue by manually specifying `undiciRequest.ts` should not be
bundled.
@suneettipirneni
fix: sleep method and root package.json
cd16789
@suneettipirneni
fix: requested changes
125e529
@suneettipirneni
fix: remove node import
6e9c4bc
@suneettipirneni suneettipirneni marked this pull request as ready for review July 4, 2023 19:37
@suneettipirneni suneettipirneni requested review from a team and iCrawl as code owners July 4, 2023 19:37
@suneettipirneni suneettipirneni requested review from ckohen, vladfrangu, kyranet, didinele and Jiralite and removed request for a team July 4, 2023 19:37
@codecov
Copy link

codecov bot commented Jul 4, 2023 •
edited
Loading

Codecov Report

Merging #9682 (e56c877) into feat/no-de-no-de (4312f63) will decrease coverage by 0.23%.
The diff coverage is 55.14%.

@@                 Coverage Diff                  @@
##           feat/no-de-no-de    #9682      +/-   ##
====================================================
- Coverage             58.03%   57.80%   -0.23%     
====================================================
  Files                   228      232       +4     
  Lines                 14955    15003      +48     
  Branches               1133     1137       +4     
====================================================
- Hits                   8679     8673       -6     
- Misses                 6236     6290      +54     
  Partials                 40       40
Flag Coverage Δ
builders 98.69% <ø> (ø)
next ∅ <ø> (∅)
proxy 76.31% <ø> (ø)
rest 92.59% <80.95%> (-0.40%) ⬇️
util 70.55% <13.46%> (-20.48%) ⬇️
ws 53.08% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/rest/src/agnostic.ts 0.00% <0.00%> (ø)
packages/rest/src/lib/CDN.ts 97.74% <ø> (-0.02%) ⬇️
packages/rest/src/web.ts 0.00% <0.00%> (ø)
packages/util/src/functions/userAgentAppendix.ts 13.46% <13.46%> (ø)
packages/rest/src/lib/RequestManager.ts 89.39% <77.77%> (-0.72%) ⬇️
packages/rest/src/environment.ts 100.00% <100.00%> (ø)
packages/rest/src/lib/REST.ts 98.87% <100.00%> (-0.04%) ⬇️
packages/rest/src/lib/errors/HTTPError.ts 100.00% <100.00%> (ø)
packages/rest/src/lib/handlers/BurstHandler.ts 91.03% <100.00%> (-0.07%) ⬇️
...ackages/rest/src/lib/handlers/SequentialHandler.ts 87.71% <100.00%> (-0.04%) ⬇️
... and 4 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

ckohen
ckohen requested changes Jul 4, 2023
View reviewed changes
packages/rest/src/lib/REST.ts Show resolved Hide resolved
packages/rest/src/lib/handlers/Shared.ts Outdated Show resolved Hide resolved
packages/rest/tsup.config.ts Outdated Show resolved Hide resolved
packages/rest/src/lib/errors/HTTPError.ts Outdated Show resolved Hide resolved
vladfrangu
vladfrangu requested changes Jul 5, 2023
View reviewed changes
packages/rest/src/lib/utils/constants.ts Outdated Show resolved Hide resolved
packages/rest/src/lib/utils/constants.ts Outdated Show resolved Hide resolved
ckohen
ckohen previously requested changes Jul 5, 2023
View reviewed changes
packages/rest/tsup.config.ts Outdated Show resolved Hide resolved
@Jiralite Jiralite removed their request for review July 6, 2023 11:31
kyranet
kyranet previously requested changes Jul 6, 2023
View reviewed changes
Copy link
Member

@kyranet kyranet left a comment

Choose a reason for hiding this comment

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

I'm also not fully convinced about the setTimeout().unref?.() since on Deno and Bun, it'll read properties that don't exist, which carries a performance penalty.

Maybe an utility named setUnrefTimeout()?

packages/rest/src/environment.ts Outdated Show resolved Hide resolved
packages/rest/src/agnostic.ts Show resolved Hide resolved
packages/rest/src/lib/RequestManager.ts Outdated Show resolved Hide resolved
packages/rest/src/lib/utils/utils.ts Outdated Show resolved Hide resolved
@ckohen
Copy link
Member

ckohen commented Jul 6, 2023

The performance impact on .unref?.() is irrelevant, it's called once on instantiation of REST (technically whenever sweepers are setup, but that's never going to be frequent)

@vladfrangu vladfrangu requested a review from kyranet July 6, 2023 22:57
kyranet
kyranet requested changes Jul 7, 2023
View reviewed changes
packages/rest/src/lib/utils/utils.ts Outdated Show resolved Hide resolved
packages/rest/src/lib/utils/constants.ts Outdated Show resolved Hide resolved
packages/rest/src/lib/REST.ts Show resolved Hide resolved
@vladfrangu vladfrangu merged commit 91f1b59 into feat/no-de-no-de Jul 9, 2023
@vladfrangu vladfrangu deleted the refactor/rest/web-compat branch July 9, 2023 11:56
vladfrangu added a commit that referenced this pull request Jul 9, 2023
@suneettipirneni @vladfrangu @Jiralite
refactor(rest): add web compatibility (#9682)
8e0be7c 
Co-authored-by: Vlad Frangu <kingdgrizzle@gmail.com>
Co-authored-by: Jiralite <33201955+Jiralite@users.noreply.github.com>
@ckohen ckohen mentioned this pull request Jul 13, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kyranet kyranet kyranet approved these changes

@vladfrangu vladfrangu vladfrangu approved these changes

@ckohen ckohen ckohen approved these changes

@iCrawl iCrawl Awaiting requested review from iCrawl

@didinele didinele Awaiting requested review from didinele didinele was automatically assigned from discordjs/rest

Assignees

@suneettipirneni suneettipirneni

Labels
packages:rest packages:util refactor semver:major
Projects
discord.js
Archived in project
Milestone
rest 2.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@suneettipirneni @ckohen @vladfrangu @kyranet @Jiralite