Skip to content

fix: typing mismatch for empty strings being treated as undefined#11287

Open
WhatCats wants to merge 1 commit intodiscordjs:mainfrom
WhatCats:fix/typing-mismatch
Open

fix: typing mismatch for empty strings being treated as undefined#11287
WhatCats wants to merge 1 commit intodiscordjs:mainfrom
WhatCats:fix/typing-mismatch

Conversation

@WhatCats
Copy link
Contributor

@WhatCats WhatCats commented Nov 20, 2025

Problem

At the moment methods like GuildMemberManager:fetch that have an optional string parameter are doing a falsy check for if the parameter was omitted. But consider the following typescript code:

const id: string = ""
const member: GuildMember = await guild.members.fetch(id)

According to the typings member should be a GuildMember but its actually a Collection<Snowflake, GuildMember> because the empty string is falsy (this is why we love JS).

My Suggestion

Do more exact undefined/null checks.
(I might not have found every place this is an issue)

@vercel
Copy link

vercel bot commented Nov 20, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
discord-js Skipped Skipped Nov 20, 2025 10:44pm
discord-js-guide Skipped Skipped Nov 20, 2025 10:44pm

@vercel vercel bot temporarily deployed to Preview – discord-js November 20, 2025 22:40 Inactive
@vercel vercel bot temporarily deployed to Preview – discord-js-guide November 20, 2025 22:40 Inactive
@WhatCats WhatCats force-pushed the fix/typing-mismatch branch from 16f28a2 to 847f9d3 Compare November 20, 2025 22:43
@vercel vercel bot temporarily deployed to Preview – discord-js-guide November 20, 2025 22:44 Inactive
@vercel vercel bot temporarily deployed to Preview – discord-js November 20, 2025 22:44 Inactive
@Jiralite Jiralite added this to the discord.js 15.0.0 milestone Nov 28, 2025
@almeidx
Copy link
Member

almeidx commented Dec 1, 2025

Personally, I think this is a step in the right direction. However, I don't think this change alone is enough, as it will potentially bring different issues. For instance, passing an empty string to RoleManager#fetch() will now make a request to the Get Guild Roles endpoint, but through the Get Guild Role path internally. This will mean the endpoint will return a different body than expected, which might create worse errors that are harder to pinpoint.

To fix that, we should make further changes here. For instance, modifying the resolveId() method of managers to return null for empty strings. This solves the issue I mentioned in most places, but there are still some places that do not make use of this method (e.g., the RoleManager), so those places would have to be modified to make use of it.

@WhatCats
Copy link
Contributor Author

WhatCats commented Dec 14, 2025

For instance, passing an empty string to RoleManager#fetch() will now make a request to the Get Guild Roles endpoint, but through the Get Guild Role path internally.

I think I understand so the original idea was that you would query the empty string with Discord but your saying since the endpoint is /guild_id/roles/role_id if you pass an empty string it would result in /guild_id/roles/ which is a different endpoint.

Side Note: This makes me think the REST package shouldn't allow empty strings so you don't accidentally call a different endpoint.

To fix that, we should make further changes here. For instance, modifying the resolveId() method of managers to return null for empty strings.

I did consider this but then you would still end up explicitly calling the fetch-all endpoint for empty strings which is a problem as I originally described.

I'm thinking maybe it would make sense to throw a Discord like error for empty strings manually (like you would get if you passed any other non-snowflake string to the API because we can't actually pass the empty string because it would be interpreted as a different endpoint).

@almeidx
Copy link
Member

almeidx commented Dec 18, 2025

I'm thinking maybe it would make sense to throw a Discord like error for empty strings manually (like you would get if you passed any other non-snowflake string to the API because we can't actually pass the empty string because it would be interpreted as a different endpoint).

We could throw an error in resolveId() if the input is a primitive value that is not a non-empty string

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

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

3 participants