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

Don't include dom types #9737

Closed
leumasme opened this issue Jul 27, 2023 · 1 comment · Fixed by #9831
Closed

Don't include dom types #9737

leumasme opened this issue Jul 27, 2023 · 1 comment · Fixed by #9831

Comments

@leumasme
Copy link

leumasme commented Jul 27, 2023

Which package is this bug report for?

discord.js

Issue description

discord.js currently includes the dom types lib in its main types.d.ts

// DOM types required for undici
/// <reference lib="dom" />

This leads to anyone importing discord.js into their project having dom types globally available (such as document, window, HTMLDivElement, ...)
These types are of course false, since most discord.js projects will run in node and not in the browser.
Looking at the comment // DOM types required for undici, it seems to reference the experimental fetch global that was recently added to nodejs.
This fetch global internally uses undici, which is what that comment is likely referencing, since the undici npm package is properly typed out of the box.

I could not find any references to the global fetch in the code from a very brief search, so I'm assuming a bit here - however since you already depend on undici anyway:

"undici": "^5.22.1",

using the global fetch would not make much sense. You are using an untyped experimental api that is only available in recent node versions, even though you have that same fetch function already installed as a dependency.

To clarify what difference this one line /// <reference lib="dom" /> makes, here are autocompletion suggestions when importing discord.js vs when not importing it:
image
image
As soon as discord.js is imported, typescript/vscode will start suggesting browser-only variables and classes. This makes this issue unavoidable, since it occours in your whole project as soon as you simply import discord.js, which is why I'd argue that this is not low priority.

Relevant:
d0c8256 by @favna introduced this issue
[A] [B] Some brief conversation regarding this issue on the discord.js discord server

Code sample

No response

Versions

  • discord.js v14.11.0
  • NodeJS v18.16.1
  • Typescript v5.1.6

Issue priority

Medium (should be fixed soon)

Which partials do you have configured?

Not applicable

Which gateway intents are you subscribing to?

Not applicable

I have tested this issue on a development release

No response

@KhafraDev
Copy link
Contributor

if this is an issue in undici i'd be glad to fix the types, why is this required?

@kodiakhq kodiakhq bot closed this as completed in #9831 Sep 11, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants