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: add support for using keyword on discord.js Client and WebSocketManager #10063

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

suneettipirneni
Copy link
Member

@suneettipirneni suneettipirneni commented Dec 29, 2023

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

This adds support for explicit resource management for the Client instances in discord.js, as well as the WebSocketManager in ws. This is done by polyfilling Symbol.dispose and Symbol.asyncDispose (if not already provided by the runtime).

Users who support the correct TypeScript project configuration can use the following syntax to clean up resources after the client goes out of scope or if it errors out:

await using client = new Client(...); 

Or if you're using the WebSocketManager from @discordjs/ws directly:

await using ws = new WebSocketManager(...);

Closes #10057

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)

Copy link

vercel bot commented Dec 29, 2023

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) Visit Preview Dec 30, 2023 6:04pm
discord-js-guide ⬜️ Ignored (Inspect) Visit Preview Dec 30, 2023 6:04pm

Copy link

codecov bot commented Dec 29, 2023

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (0f1e02b) 58.36% compared to head (e4bff42) 58.35%.
Report is 3 commits behind head on main.

Files Patch % Lines
packages/util/src/functions/polyfillDispose.ts 42.85% 8 Missing ⚠️
packages/ws/src/ws/WebSocketManager.ts 77.77% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10063      +/-   ##
==========================================
- Coverage   58.36%   58.35%   -0.01%     
==========================================
  Files         242      243       +1     
  Lines       17184    17206      +22     
  Branches     1243     1243              
==========================================
+ Hits        10029    10041      +12     
- Misses       7110     7120      +10     
  Partials       45       45              
Flag Coverage Δ
builders 95.53% <ø> (ø)
next ∅ <ø> (∅)
proxy 75.00% <ø> (ø)
rest 92.87% <ø> (ø)
util 68.86% <42.85%> (-1.84%) ⬇️
ws 52.84% <77.77%> (+0.08%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Jiralite Jiralite added this to the discord.js 14.15 milestone Dec 29, 2023
Copy link
Member

@didinele didinele left a comment

Choose a reason for hiding this comment

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

Could you maybe look into doing this in WS as well? WebSocketManager specifically

@suneettipirneni suneettipirneni changed the title feat: add support for using keyword on Client feat: add support for using keyword on discord.js Client and WebSocketManager Dec 29, 2023
@suneettipirneni suneettipirneni requested a review from a team as a code owner December 29, 2023 20:04
@suneettipirneni suneettipirneni self-assigned this Dec 29, 2023
@Jiralite Jiralite removed their request for review December 29, 2023 20:49
@Jiralite Jiralite added blocked and removed blocked labels Dec 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Review in Progress
Development

Successfully merging this pull request may close these issues.

BaseClient/Client: Add support for 'using' keyword (Explicit Resource Management)
6 participants