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

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

Open
StarManTheGamer opened this issue Jun 20, 2024 · 14 comments · May be fixed by #10360
Open

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

StarManTheGamer opened this issue Jun 20, 2024 · 14 comments · May be fixed by #10360

Comments

@StarManTheGamer
Copy link

StarManTheGamer commented Jun 20, 2024

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
Copy link
Author

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
Copy link
Member

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

@JstnMcBrd
Copy link

JstnMcBrd commented Jun 21, 2024

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

However, [email protected] was released today, so that lines up much better with the time frame.

@JstnMcBrd
Copy link

I also encountered this issue when type-checking my personal project that uses discord.js after updating to [email protected].

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
Copy link
Member

I found microsoft/TypeScript#58907 earlier, but I was not sure if it was related.

@JMTK
Copy link
Contributor

JMTK commented Jun 21, 2024

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
Copy link
Member

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
Copy link
Contributor

Renegade334 commented Jun 21, 2024

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
Copy link
Author

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
Copy link
Member

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
Copy link
Contributor

Renegade334 commented Jun 21, 2024

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
Copy link
Contributor

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

@panalgin
Copy link

as a temporary workaround:

npm i -D [email protected]
yarn add [email protected] --dev

prevents EventEmitter already defined errors in the project

@JstnMcBrd
Copy link

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.

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

Successfully merging a pull request may close this issue.

8 participants