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

Homogenise null and undefined typings within djs APIs #9829

Open
noftaly opened this issue Sep 11, 2023 · 3 comments
Open

Homogenise null and undefined typings within djs APIs #9829

noftaly opened this issue Sep 11, 2023 · 3 comments

Comments

@noftaly
Copy link
Contributor

noftaly commented Sep 11, 2023

Which application or package is this feature request for?

discord.js

Feature

Often when using discord.js APIs together, types are incompatible because of undefined/null discrepancies. This is very irritating and hurts DX because you would expect that using discord.js APIs together would "just work" and would not require you to resort to hacks.
Examples

  1. Embed Builder:
new EmbedBuilder()
  .setAuthor({
    name: myText,
    iconURL:     member.user.avatarURL(),
//  ^^^^^^^                  ^^^^^^^^^^^
//  string | undefined       string | null
  })
  1. Last Message ID
 await message.channel.messages.fetch({
   before: message.channel.lastMessageId
// ^^^^^^                  ^^^^^^^^^^^^^
// string | undefined      string | null
 })

Probably many other ones. Those were just examples, the main problem is that for class properties | null is mainly used, but for option objects prop?: type is mainly used, which makes it undefined | type.

This is a problem because it results in a Typescript compiler error.

Ideal solution or implementation

The easiest option would be to have all Options objects with optional properties to also accept nulls, which afaict wouldn't be breaking (?) because you don't explicitly check for undefined values.

I haven't looked in other places, but at least accepting nulls in option objects would probably remove 95% of the problems (or of the common problems) faced by djs users.

Alternative solutions or implementations

You could also just ignore this problem as it is easily fixable by asserting not-null (!) to make Typescript happy, or by adding ?? undefined which would also fix the problem by making Typescript happy with a solution that makes it through runtime and not just compile time.

Other context

Debates over the use of null / undefined:

Utilities from Sapphire to handle both undefined/null together (aka Nullish) : isNullish and Nullish type alias

@didinele
Copy link
Member

👎 Most of those type choices are deliberate, especially when it comes to patching data. undefined/missing implies no changes are meant for the property, while null can have special functionality.

I suppose there are cases and structures where this isn't necessarily a problem, but I feel like all things considered, it'd be a downgrade to change it in specific areas and cause an inconsistency all together.

@noftaly
Copy link
Contributor Author

noftaly commented Sep 11, 2023

I know that and completely agree, but i don't think that (generally speaking from the patterns i could see) returning null from getters/methods and accepting undefined in option objects/inputs is a viable option. I definitely hurts user experience and end-code readability in many ways. The solution for that isn't perfect but I do believe it is a small price to pay for DX.

The single source of truth with the deliberate choices you are talking about could still be the docs or dapi-types, but the user-facing API could be polished by abstracting the null/undefined differences away.

@didinele
Copy link
Member

It's unclear to me how much we can improve this without compromising functionality regardless, but returning undefined where possible is on the table and something we've been thinking of.

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

No branches or pull requests

3 participants