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

Major: discordjs/rest crashing on Node v16.9.1 #9745

Closed
maxtull opened this issue Aug 1, 2023 · 14 comments
Closed

Major: discordjs/rest crashing on Node v16.9.1 #9745

maxtull opened this issue Aug 1, 2023 · 14 comments

Comments

@maxtull
Copy link

maxtull commented Aug 1, 2023

Which package is this bug report for?

rest

Issue description

Require Discord.JS (v14.12.1) in a javascript file then attempt to run that file using Node. This has only started occuring with the upgrade from DJS v14.11.0 to v14.12.

Code sample

There is no code sample provided due to this issue occuring when importing (requiring) the package (discord.js).

4|PDB    | /home/ubuntu/PDB/node_modules/@discordjs/rest/dist/index.js:175
4|PDB    |   static {
4|PDB    |          ^
4|PDB    | SyntaxError: Unexpected token '{'
4|PDB    |     at Object.compileFunction (node:vm:352:18)
4|PDB    |     at wrapSafe (node:internal/modules/cjs/loader:1031:15)
4|PDB    |     at Module._compile (node:internal/modules/cjs/loader:1065:27)
4|PDB    |     at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
4|PDB    |     at Module.load (node:internal/modules/cjs/loader:981:32)
4|PDB    |     at Function.Module._load (node:internal/modules/cjs/loader:822:12)
4|PDB    |     at Module.require (node:internal/modules/cjs/loader:1005:19)
4|PDB    |     at Module.Hook._require.Module.require (/home/ubuntu/.nvm/versions/node/v16.9.1/lib/node_modules/pm2/node_modules/require-in-the-middle/index.js:101:39)
4|PDB    |     at require (node:internal/modules/cjs/helpers:94:18)
4|PDB    |     at Object.<anonymous> (/home/ubuntu/PDB/node_modules/discord.js/src/client/BaseClient.js:4:18)

Versions

@discordjs/opus - 0.9.0
@discordjs/voice - 0.16.0
discord.js - 14.12.1
Node - 16.9.1

Issue priority

High (immediate attention needed)

Which partials do you have configured?

No Partials

Which gateway intents are you subscribing to?

Guilds, GuildMembers, GuildEmojisAndStickers, GuildMessages, GuildMessageReactions, DirectMessages, DirectMessageReactions, MessageContent

I have tested this issue on a development release

[email protected]

@maxtull
Copy link
Author

maxtull commented Aug 1, 2023

Updated to Node v18 as specified in the rest 2.0.0 milestone changelog.

@maxtull maxtull closed this as completed Aug 1, 2023
@maxtull maxtull reopened this Aug 1, 2023
@iCrawl
Copy link
Member

iCrawl commented Aug 1, 2023

16.11 is fine too, and shouldn't be too problematic for now.

@maxtull
Copy link
Author

maxtull commented Aug 1, 2023

Reopened because this affects Discord.JS directly itself when updating from 14.11.* to 14.12.1.

@maxtull
Copy link
Author

maxtull commented Aug 1, 2023

16.11 is fine too, and shouldn't be too problematic for now.

Ah cheers, I'll downgrade to 16.11. Might wanna update docs if possible, thanks for the quick reply.

@ckohen
Copy link
Member

ckohen commented Aug 1, 2023

FYI, the Node 18 requirement was reverted as to avoid a major bump in discord.js, and as crawl said, this feature was added in Node 16.11 which I think is a reasonable enough expectation to update to when updating dependencies. Will work on a solution to either fix the way tsc compiles this or document it.

@ckohen ckohen closed this as completed Aug 1, 2023
@ckohen ckohen reopened this Aug 1, 2023
@GodderE2D
Copy link
Contributor

FYI, the Node 18 requirement was reverted as to avoid a major bump in discord.js, and as crawl said, this feature was added in Node 16.11 which I think is a reasonable enough expectation to update to when updating dependencies. Will work on a solution to either fix the way tsc compiles this or document it.

In this case, docs should be updated to reflect this, right?

@sdanialraza
Copy link
Contributor

The 2.0.0 release says 18+ is required because of the use of global fetch so a bit confused, is it 16.11+ or 18+?

@GodderE2D
Copy link
Contributor

I'm not too sure but this workaround/polyfill should work for 16.11. So until v15, it should be 16.11+.

https://github.com/discordjs/discord.js/blob/main/packages/rest/src/index.ts#L7

@ckohen
Copy link
Member

ckohen commented Aug 1, 2023

That breaking change comment was left in because that commit had 4 other breaking changes, the revert of the engine requirement bump happened later and the changelog generator did not pick it up for some reason.

Also, the reason it was reverted in the first place is that the bump was not actually necessary since we don't use global fetch in node, only in web / web-like environments.

@maxtull
Copy link
Author

maxtull commented Aug 1, 2023

Just downgraded to 16.11 and the issue is still occuring?

@ckohen
Copy link
Member

ckohen commented Aug 1, 2023

To be clear, the same issue is occurring?

The parsing of this syntax was added in v8 9.4.146, which Node updated to in 16.11.0. I also tested this with a minimum reproducible sample in 16.10 and 16.11 to confirm this, and the sample ran fine on 16.11 as expected.

@maxtull
Copy link
Author

maxtull commented Aug 1, 2023

I removed node_modules and ran npm install targetting Discord.JS v14.12.1 (Also upgraded to Node v16.13.0). The issue still occured when the project was ran, however this may have been caused by NPM caching packages - I didn't think of that at the time but thats likely what it was. I have reverted to v14.11 for now and will wait for further updates.

@ckohen
Copy link
Member

ckohen commented Aug 1, 2023

I made a fresh package, installed discord.js and created a file that only imported discord.js, it crashed on 16.10 as expected, and ran fine on 16.11 in both cjs and esm.

If you are using some type of process manager, make sure it is updating when you update your global version of node, especially if you have multiple versions of node installed at the same time.

@maxtull
Copy link
Author

maxtull commented Aug 8, 2023

Inactive, issue resolved with Node v16.11+. Docs may need to be updated to include the version bump (assuming they are not already). Thanks for the help. 👍

@maxtull maxtull closed this as completed Aug 8, 2023
@Jiralite Jiralite closed this as not planned Won't fix, can't repro, duplicate, stale Aug 8, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants