-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat(semver-major): Upgrade package to Node.js v23 #54
base: main
Are you sure you want to change the base?
Conversation
ea8ee3f
to
90713b6
Compare
with: | ||
node-version: ${{ matrix.node }} | ||
node-version: 18 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's bring back the matrix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.This package isn't compatible with v20, or with v16. It's specifically meant for v18
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably be aiming for support on all supported versions of Node.js. If there's something that makes it work only for v18 but not v20, something's probably wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look into it, but we really don't need to support v20. v20 has most features that this aims to replicate
This PR is hard to review without knowing what process did you follow and understanding the motivations behind the choices that were made. |
Sorry if this is difficult to review! A breakdown of my thought process is as follows: When updating the library, there was the primary goal of adding as many v23 features that could be backported to this version easily. Along with this goal, a secondary goal was to modify as little source code as possible (E.G. the files in With that in mind, the instances where code absolutely had to be modified are denoted with Additionally, I understood the following limitations
Anything else you want to know? |
(Edit by @RedYetiDev: I accidentally removed the content of this comment, re-added) I think landing this is good as a way to get close to catching up, but ideally there would be 1:1 parity between the package and the core module - it should be a drop in replacement. IMO, testing shouldn't be using core modules - we should only be using things that are available to the ecosystem. If there's something that's useful for testing that we want to hook into in core, we should find ways to expose those APIs. |
I highly disagree with this. The test runner should make use of core modules, it's a builtin for a reason. Users who do not wish for this behavior can use alternative libraries, such as Mocha. As for the builtin, using internal modules allows for it to use a wide-range of features not typically exposed to userland, and it can access (if needed) the CPP builtins that are the backbone of Node.js. |
Ref: nodejs/admin#928
This PR introduces a major upgrade to the package, adding support for Node.js v23 features. Please note that the minimum required Node.js version is now v18, which was used for all testing.
The update includes most of the new features in v23, with the following exceptions:
node_modules
located outside the test module's directory--require
or--import
when process isolation is disabled and multiple files need to be executed