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: use cache.get() for snowflakes, resolve() otherwise #10626

Merged
merged 4 commits into from
Nov 28, 2024
Merged

refactor: use cache.get() for snowflakes, resolve() otherwise #10626

merged 4 commits into from
Nov 28, 2024

Conversation

sdanialraza
Copy link
Contributor

@sdanialraza sdanialraza commented Nov 28, 2024 •
edited
Loading

Please describe the changes this PR makes and why it should be merged:
This makes it consistent to use cache.get() for snowflakes and resolve() for everything else.

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

@sdanialraza sdanialraza requested a review from a team as a code owner November 28, 2024 13:38
@vercel Vercel
Copy link

vercel bot commented Nov 28, 2024 •
edited
Loading

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

2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
discord-js ⬜️ Ignored (Inspect) Visit Preview Nov 28, 2024 5:44pm
discord-js-guide ⬜️ Ignored (Inspect) Visit Preview Nov 28, 2024 5:44pm

didinele
didinele previously approved these changes Nov 28, 2024
View reviewed changes
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.

surely this won't somehow break anything

@sdanialraza sdanialraza force-pushed the refactor/consistent-use-resolve-cache-get branch from b63cdb3 to 43a7060 Compare November 28, 2024 15:39
@sdanialraza sdanialraza force-pushed the refactor/consistent-use-resolve-cache-get branch from 43a7060 to 6b65bad Compare November 28, 2024 15:50
@sdanialraza sdanialraza changed the title refactor: consistent use of resolve instead of cache.get() ?? null refactor: use cache.get() for snowflakes, resolve() otherwise Nov 28, 2024
@sdanialraza sdanialraza requested a review from didinele November 28, 2024 15:55
@Jiralite Jiralite added this to the discord.js 15.0.0 milestone Nov 28, 2024
@sdanialraza @Jiralite
fix: requested changes
dd19a66 
Co-authored-by: Jiralite <[email protected]>
@sdanialraza sdanialraza requested a review from Jiralite November 28, 2024 17:00
@kodiakhq kodiakhq bot merged commit a870bc1 into discordjs:main Nov 28, 2024
7 checks passed
@sdanialraza sdanialraza deleted the refactor/consistent-use-resolve-cache-get branch November 29, 2024 14:46
Jiralite added a commit that referenced this pull request Dec 5, 2024
@sdanialraza @Jiralite @kodiakhq
refactor: use cache.get() for snowflakes, resolve() otherwise (#1…
dedaa5d 
…0626)

* refactor: use `cache.get()` for snowflakes, `resolve()` otherwise

* fix: requested changes

Co-authored-by: Jiralite <[email protected]>

* chore: remove unnecessary `?? null`

---------

Co-authored-by: Jiralite <[email protected]>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
@DJj123dj
Copy link

DJj123dj commented Jan 3, 2025

This change DID indeed break a lot of bots. It might not look like it at a first glance, but the cache.get() method only allows ID's/snowflakes. It returns undefined when trying to resolve a Role or User class in the PermissionOverwrites.

The param type still allows the Role & User class to be used even if they aren't processed and will just return undefined.

😄 But no worries, in the latest version of discord.js is this code already overwritten/modified & now it works again.

@Jiralite
Copy link
Member

Jiralite commented Jan 3, 2025

Code talks better! What was the issue? I want to understand what you are talking about.

@sdanialraza sdanialraza mentioned this pull request Jan 4, 2025
Merged
@DJj123dj
Copy link

DJj123dj commented Jan 4, 2025

Code talks better! What was the issue? I want to understand what you are talking about.

Yea, the issue was exactly what is described in pull request #10686 as mentioned by @sdanialraza. The fact that it would return undefined when using a discorde.Role or discord.User in the id of the PermissionOverwrites instead of a string (snowflake)

@Jiralite
Copy link
Member

Jiralite commented Jan 4, 2025

We had decoded your message by then! Please open an issue instead of commenting on pull requests in the future with a reproducible code sample next time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jiralite Jiralite Jiralite approved these changes

@SpaceEEC SpaceEEC SpaceEEC approved these changes

@vladfrangu vladfrangu vladfrangu approved these changes

@iCrawl iCrawl Awaiting requested review from iCrawl iCrawl is a code owner automatically assigned from discordjs/core

@kyranet kyranet Awaiting requested review from kyranet kyranet is a code owner automatically assigned from discordjs/core

@didinele didinele Awaiting requested review from didinele

Assignees
No one assigned
Labels
packages:discord.js refactor semver:patch
Projects
discord.js
Archived in project
Milestone
discord.js 15.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@sdanialraza @DJj123dj @Jiralite @vladfrangu @SpaceEEC @didinele