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

Update to Node 14 #1253

Open
LJNeon opened this issue Jul 28, 2021 · 13 comments
Open

Update to Node 14 #1253

LJNeon opened this issue Jul 28, 2021 · 13 comments

Comments

@LJNeon
Copy link
Contributor

LJNeon commented Jul 28, 2021

Eris currently has a minimum supported version of Node 10. However that version was released in 2018, isn't maintained anymore, and doesn't receive security patches. With Node 16 entering LTS in less than 3 months, I believe we should bump the minimum supported version to at least Node 14.

Updating to Node 14 brings countless security and performance improvements (especially to JSON parsing), bug fixes, and useful new features such as:

I am willing to make a PR for this, however it's practically guaranteed to be full of merge conflicts with any existing PRs due to these new features. Because of that, I believe it would be better to wait until after the next Eris release to open such a PR, as merge conflicts will be at their lowest then.

@abalabahaha
Copy link
Owner

abalabahaha commented Aug 19, 2021

  • Security and performance: IMO alone, this isn't enough reason, unless the Node version seriously breaks the lib (AFAIK not the case for Node 10 + Eris). The user is free to use whatever Node version Eris runs on. If the user needs/wants an older Node version and is willing to forego recent security and performance improvements, it's not the job of a Discord library to convince them otherwise.
  • Nicer code: is a better reason, sure

I'm fine with new commits targeting minimum active LTS (currently Node 14), and then backporting big fixes to maintenace LTS versions (currently Node 12).

@bsian03
Copy link
Collaborator

bsian03 commented Aug 19, 2021

Sounds like something for a new branch

@DonovanDMC

This comment has been minimized.

@abalabahaha
Copy link
Owner

As said in OP,

I believe it would be better to wait until after the next Eris release to open such a PR

Also the next lib update has a bunch of features and I think everything already runs fine on Node 10, I'd rather nobody go out of their way to break compatibility.

that sounds like an overcomplicated mess

We already have version branches (e.g. 0.14.x) that get occasional backports. Changing supported Node version would be semver bump, so same thing.

@HarbingerNight
Copy link
Contributor

It should be noted that erlpack isn't even capable of compiling w/ Node 16 support.

@DonovanDMC
Copy link
Contributor

It should be noted that erlpack isn't even capable of compiling w/ Node 16 support.

We wouldn't be going to v16 for a long while, and losing erlpack isn't really that bad of a loss

@GoldenAngel2
Copy link

It supports node 16, the pr for it got merged: discord/erlpack#41

@retraigo
Copy link
Contributor

This could be a good first step towards moving from CJS to ESM. ES Modules have a number of advantages over the current CommonJS, mainly the following:

  • Named exports. These have already been resolved through the esm.mjs file so I'ma ignore this.
  • Async loading. ES Modules are loaded asynchronously compared to CommonJS' synchronous loading with require(). More speed is good speed.
  • Top-level await. Since ES Modules load asynchronously, they allow you to use await at the top level of the module.

However, these would be severely breaking changes due to the inability of CommonJS projects to load ES Modules. Therefore, these changes can definitely not be made without a warning. But they're a change certainly worth considering.

@DonovanDMC
Copy link
Contributor

DonovanDMC commented Feb 25, 2022

Considering how insane abal is about backwards compatibility, I seriously doubt eris will ever go full esm for the exact stated reason at the bottom - I'd take a good bet eris would lose a LOT of its userbase to a change like that

and unless there's something different about it.. commonjs can do named exports?

// 1.js
module.exports.whatever = "hi";
// 2.js
const { whatever } = require("./1");
console.log(whatever); // hi

@retraigo
Copy link
Contributor

retraigo commented Feb 25, 2022

CJS exports are... One single object. There is one object exported and that is module.exports. Any extra named export you make goes into module.exports.

ESM exports are different. The default export and the named exports have nothing to do with each other.

export {no, strangers, to, love, know, the, rules}
export default {
    were: {
        no,
        strangers,
        to,
        love,
    },
    you: {
        know,
        the,
        rules,
    }
}

The two export statements do not interfere with each other. They're exported like,

{
    ...namedExports,
    default
}
import x from 'this file';
// you get that giant object from `default`
import {strangers, love} from 'this file';
// you directly get the two from the named exports.

The main advantage is being able to export whatever you want at the top level for users to directly import them.

Tho yea I do agree that going full ESM suddenly would just crumble the userbase. However, a lot of popular small modules are already going full ESM. A good example would be node-fetch. A complete change is just in the near future. I ain't asking to just suddenly move to ESM. I'm just asking to keep it in the plans moving forward.

The main reason a lot of people give for not switching over is "Typescript and Electron do not support ESM yet". When the large modules like these make the move, it'll just be a matter of time before the majority start using ESM.

@eritbh
Copy link
Contributor

eritbh commented Feb 25, 2022

I don't think anyone is debating that CJS and ESM are structured differently. However, like you said in your first comment, Eris already has explicit support for ESM consumers by providing an ESM entry point that uses named exports. The distinction between the default export and named exports in ESM is unlikely to matter to internal code, since I don't think we have any file (other than the main entry point) where a non-trivial default export is used at the same time as named exports.

Similarly, top-level await isn't an advantage to Eris internally because the library has no need for it. Consumers of the library can already take advantage of that feature by importing Eris into an ESM context themselves. The only distinct advantage to be gained by refactoring Eris's own code to ESM would be any performance gained by the new module loading system, and for the vast majority of cases where Eris is loaded once on process startup, that seems unlikely to have real-world impacts to users either. So stable ESM support doesn't seem like a reason to drop support for older versions of Node on its own.

@elenakrittik
Copy link

Node.js released v20 recently. Abal, are you really sure there is a reason to support v10? It reached EOL whole 2 years ago, and v14 reached EOL recently too. If you care about your userbase, then simply make a poll asking what nodejs version people use. I'm pretty sure there won't be much answers below v14.

@abalabahaha
Copy link
Owner

are you really sure there is a reason to support v10? It reached EOL whole 2 years ago.

@ItsAleph that comment is also from over 1.5 years ago. If you update the numbers, the general sentiment of the comment still stands.

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

9 participants