discordjs / discord.js

A powerful JavaScript library for interacting with the Discord API
https://discord.js.org
Apache License 2.0
25.1k stars 3.94k forks source link

Conflict between discord.js + @nodes/types after upgrading #10358

Open StarManTheGamer opened 3 weeks ago

StarManTheGamer commented 3 weeks ago

Which package is this bug report for?

discord.js

Issue description

Hi there, approximately two hours ago @nodes/types upgraded their package. I use GitHub Actions to build and test my discord bot, and I didn't realize I was out-of-date until I got conflicting errors that were very confusing.

As far as I can tell, I believe that @ nodes/types did some important changes and now discord.js is conflicting with those changes, and this is an issue with discord.js but I'm not sure. I'm not overly a huge expert on these types of things.

You can replicate this issue by installing the latest @nodes/types and the latest development build of discord.js. Upon attempting to build your project, you will be greeted with this error:

Error: node_modules/@types/node/events.d.ts(519,30): error TS2300: Duplicate identifier 'EventEmitter'.
Error: node_modules/@types/node/stream.d.ts(53,11): error TS2420: Class 'ReadableBase' incorrectly implements interface 'ReadableStream'.
  Type 'ReadableBase' is missing the following properties from type 'ReadableStream': off, removeAllListeners, setMaxListeners, getMaxListeners, and 4 more.
Error: node_modules/@types/node/stream.d.ts(662,11): error TS2420: Class 'WritableBase' incorrectly implements interface 'WritableStream'.
  Type 'WritableBase' is missing the following properties from type 'WritableStream': off, removeAllListeners, setMaxListeners, getMaxListeners, and 4 more.
Error: node_modules/@types/node/test.d.ts(380,11): error TS2420: Class 'TestsStream' incorrectly implements interface 'ReadableStream'.
  Type 'TestsStream' is missing the following properties from type 'ReadableStream': off, removeAllListeners, setMaxListeners, getMaxListeners, and 4 more.
Error: node_modules/discord.js/typings/index.d.ts(246,[9](https://github.com/Polytoria/CommunityBot/actions/runs/9605412040/job/26492987136#step:6:10)): error TS2300: Duplicate identifier 'EventEmitter'.
Error: Process completed with exit code 2.

Code sample

// <reference path="index.d.ts"/>
import { Client, GatewayIntentBits, ActivityType, Events, Collection, BaseInteraction } from 'discord.js'
import dotenv from 'dotenv'
import { success, warning } from './utils/log.js'
import commandsData from './commandsData.js'
import { IConfiguration } from '../types'

// Initialize .env file.
dotenv.config()

const configuration: IConfiguration = {
  token: process.env.TOKEN,
  clientId: process.env.CLIENTID,
  coolDown: 3
}

const client = new Client({
  intents: [
    GatewayIntentBits.GuildMessages,
    GatewayIntentBits.Guilds,
    GatewayIntentBits.GuildMembers,
    GatewayIntentBits.MessageContent
  ]
})

// @ts-expect-error
client.commands = new Collection()

commandsData.forEach((commandData, index) => {
  // @ts-expect-error
  client.commands.set(commandData.data.name, commandData)
})

client.on('ready', () => {
  // @ts-expect-error
  client.user.setActivity(`${client.guilds.cache.reduce((a, guild) => a + guild.memberCount, 0)} Users`, { type: ActivityType.Watching })

  setInterval(function () {
    // @ts-expect-error
    client.user.setActivity(`${client.guilds.cache.reduce((a, guild) => a + guild.memberCount, 0)} Users`, { type: ActivityType.Watching })
  }, 60000)
})
success({ context: '[Bot]', message: 'Bot succesfully started.' })

client.on('messageCreate', async (message) => {
  if (message.author.bot) return
  if (!message.content.startsWith('p!')) return
  if (!message.inGuild) return

  await message.reply('The Polytoria Community Bot has switched to slash commands!')
})

client.on(Events.InteractionCreate, async (interaction:BaseInteraction) => {
  if (!interaction.isCommand()) {
    return
  }

  // @ts-expect-error
  const command:any = interaction.client.commands.get(interaction.commandName)

  if (!command) {
    interaction.reply("Command doesn't exist")
    return
  }

  success({
    context: '[Client]',
    message: 'Command registered.'
  })

  success({
    context: '[Bot]',
    message: 'Running command ' + command.data.name
  })

  try {
    if (command.constructor.name === 'AsyncFunction') {
      await command.execute(interaction)
    } else {
      command.execute(interaction)
    }
  } catch (error: any) {
    if (interaction.replied) {
      await interaction.followUp('Failed to execute command: ' + error)
    } else {
      await interaction.reply('Failed to execute command: ' + error)
    }
    warning({
      context: '[Bot]',
      message: error.toString()
    })
  }
})

// Handle Promise Rejection
process.on('unhandledRejection', (reason, p) => {
  console.error(reason, 'Unhandled Rejection at Promise', p)
})

process.on('uncaughtException', err => {
  console.error(err)
  process.exit(1)
})

success({ context: '[Bot]', message: 'Bot succesfully logged in.' })

client.login(configuration.token)

Versions

typescript - 5.5.2 discord-js - latest development build @types/node - 20.14.7

Node - Latest v20

Issue priority

High (immediate attention needed)

Which partials do you have configured?

No Partials

Which gateway intents are you subscribing to?

Guilds, GuildMembers, GuildMessages, MessageContent

I have tested this issue on a development release

No response

StarManTheGamer commented 3 weeks ago

Upon more investigating, by removing this class from the script it resolved the issues. This is why I believe this is an issue with discord.js image

Jiralite commented 3 weeks ago

A quick inspection leads to this being reproducible only with TypeScript 5.5.2.

JstnMcBrd commented 3 weeks ago

Another quick inspection shows that this is not related to the @types/node update. The problem persists, even going back as far as @types/node@20.14.7, which was released 2 months ago.

However, typescript@5.5.2 was released today, so that lines up much better with the time frame.

JstnMcBrd commented 3 weeks ago

I also encountered this issue when type-checking my personal project that uses discord.js after updating to typescript@5.5.2.

After further investigation, I found that TS 5.5 throws errors for module augmentation, which is what you are doing to the EventEmitter class in typings/index.d.ts. I was able to reproduce the problem outside of discord.js by trying to augment EventEmitter myself. I'm still investigating whether it is specific to @types/node or affects all augmentation in general.

I can use skipLibCheck in my personal project as a workaround, but that will not work for building discord.js itself, should you choose to update to TS 5.5.

I don't see a bug report in the TypeScript repo yet, so I've been doing more researching and thinking about submitting one.

Jiralite commented 3 weeks ago

I found https://github.com/microsoft/TypeScript/issues/58907 earlier, but I was not sure if it was related.

JMTK commented 3 weeks ago

Yeah it's happening outside of discord.js, I can't seem to myclass extends EventEmitter anywhere in TypeScript 5.5 unless I explicitly define the .on in my interface like:

  public on<Event extends keyof ClientEvents>(event: Event, listener: (...args: ClientEvents[Event]) => void): this;
vladfrangu commented 3 weeks ago

This sounds more like a TS 5.5.2 issue, which I'd recommend opening an issue on their side (if one wasn't opened already)

Renegade334 commented 3 weeks ago

Classes are explicitly unmergeable; this only worked previously due to a specific bug related to merging aliased symbols, which was patched in microsoft/TypeScript#58326. The error here is unfortunately intended behaviour.

Methods on the class prototype are overloadable via interface merging, but as things stand, static methods on the constructor aren't.

StarManTheGamer commented 3 weeks ago

Classes are explicitly unmergeable; this only worked previously due to a specific bug related to merging aliased symbols, which was patched in microsoft/TypeScript#58326. The error here is unfortunately intended behaviour.

The class prototype is extensible via interface merging, but as things stand, the constructor isn't. Unfortunately, this means no ability to add or extend class static methods.

So is there a better solution to prevent the errors?

vladfrangu commented 3 weeks ago

On our side, we could use interface merging, but that has its own can of worms. We probably want to take a look at adding these extensions only on the client(s)

Renegade334 commented 3 weeks ago

There is an alternative approach, I suppose. Client.on() and Client.once() are currently the same as EventEmitter.on() and EventEmitter.once() via inheritance, but maybe those could be overridden with the type-safe signatures:

export class Client<Ready extends boolean = boolean> extends BaseClient {
  // ...

  // Override static EventEmitter methods, with added type checks for Client events.
  public static once<Emitter extends EventEmitter, Event extends keyof ClientEvents>(
    eventEmitter: Emitter,
    eventName: Emitter extends Client ? Event : string | symbol,
  ): Promise<Emitter extends Client ? ClientEvents[Event] : any[]>;
  public static on<Emitter extends EventEmitter, Events extends keyof ClientEvents>(
    eventEmitter: Emitter,
    eventName: Emitter extends Client ? Events : string | symbol,
  ): AsyncIterableIterator<Emitter extends Client ? ClientEvents[Events] : any[]>;
}

This would provide type-safe versions of those methods, without needing to interfere with EventEmitter.

(vlad: I think this is probably the sort of thing you were alluding to in your comment?)

RedGuy12 commented 3 weeks ago

See #7986 for why this augmentation was added in the first place - Client#on() should work the same even without it.

panalgin commented 3 weeks ago

as a temporary workaround:

npm i -D typescript@5.4.5 yarn add typescript@5.4.5 --dev

prevents EventEmitter already defined errors in the project

JstnMcBrd commented 3 weeks ago

For future reference:

A bug report was submitted in the TypeScript repo, and the maintainers confirmed that this is intended behavior - class augmentations cannot be merged.

StarManTheGamer commented 2 weeks ago

Any update on this issue? I was hoping to have a fix soon so I could update my typescript.

Tomato6966 commented 1 week ago

Any update on this issue? I was hoping to have a fix soon so I could update my typescript.

They made a pr which solves the issue, still waiting for merge, but you can install the commit manually by doing: https://github.com/discordjs/discord.js/pull/10360

to install the pr correctly, first: uninstall discord.js

npm uninstall discord.js

second: add the following to your package.json (to still use it as "discord.js"

"resolutions": {
    "discord.js": "github:discordjs/discord.js#8dd69cf2811d86ed8da2a7b1e9a6a94022554e96"
},

third: then install it by doing

npm i discordjs/discord.js#8dd69cf2811d86ed8da2a7b1e9a6a94022554e96

sometimes you have to call npm i again to make sure your resolutions work

Alternative: Manually inserting the correct types

you can also do the following instead of installing the fix of pr, while beeing on the latest ts version

class yourCustomEventEmitterClass extends EventEmitter implements NodeJS.EventEmitter {
}

this will work fine for types

Qjuh commented 1 week ago

I‘d still suggest not doing too many manual workarounds like that. Instead wait for a proper fix, which will probably be in @types/node to support fully typed EventEmitter. Then discord.js can just drop the module augmentation altogether and it will „just work“™️.

When that happens the linked PR would be obsolete too.

RedGuy12 commented 1 week ago

I don't believe using a resolution like that would work since this is a monorepo...

ZatsuneNoMokou commented 3 days ago

Another workaround is to use https://yarnpkg.com/package?name=eventemitter2 which is compatible with node's EventEmitter like code