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.

Sign up for GitHub

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

Jump to bottom

Require Node 16+ while building/testing on Node 20 #596

Merged
merged 3 commits into from Sep 26, 2023
Merged

Require Node 16+ while building/testing on Node 20 #596

merged 3 commits into from Sep 26, 2023

Conversation

chadlwilson
Copy link
Contributor

@chadlwilson chadlwilson commented Sep 23, 2023 •
edited

Node 16 is nearly EOL, and Node 20 is nearly LTS, so let's go straight there.

This PR also changes the way the plugin installs via gauge to use --omit=dev which is preferred above --production, which requires npm v7+. Since npm 7+ comes with NodeJS 15+ and NodeJS 16 is now EOL, this shouldn't be a concern, but bumping the plugin major version anyway, since this is technically not backward compatible.

@chadlwilson chadlwilson added the ReleaseCandidate If a PR is tagged with this label, after merging it will be released label Sep 23, 2023
@gaugebot
Copy link

gaugebot bot commented Sep 23, 2023

@chadlwilson Thank you for contributing to gauge-js. Your pull request has been labeled as a release candidate 🎉🎉.

Merging this PR will trigger a release.

Please bump up the version as part of this PR.

Instructions to bump the version can found at CONTRIBUTING.md

If the CONTRIBUTING.md file does not exist or does not include instructions about bumping up the version, please looks previous commits in git history to see what changes need to be done.

@chadlwilson
Use canonical encoding/readFile command to take advantage of fast pat…
97cfa57 
…h in Node 20

Fast path doesn't appear to check encoding options case insensitive, 'utf8' seems more canonical.

Signed-off-by: Chad Wilson <chadw@thoughtworks.com>
@chadlwilson chadlwilson self-assigned this Sep 25, 2023
@chadlwilson chadlwilson changed the title Migrate build to Node 20 Require Node 16+ with build on Node 20 Sep 25, 2023
@chadlwilson chadlwilson marked this pull request as draft September 25, 2023 06:44
@chadlwilson
Copy link
Contributor Author

Currently I realised there is a problem with the use of --omit=dev via prepare or postinstall npm scripts due to the way the plugin is installed on user machines via gauge so might need to find another way to address the necessary patch of mock-fs that avoids the use of npm scripts, so converted back to draft.

@chadlwilson
Apply workaround for mock-fs Node 20 issue
39b152f 
See tschaub/mock-fs#377

Signed-off-by: Chad Wilson <chadw@thoughtworks.com>
@chadlwilson
Migrate build to Node 20
ea9086a 
Node 16 is nearly EOL, and Node 20 is nearly LTS, so let's go straight there. This remove use of a deprecated npm flag, replacing with alternative compatible with npm 7/NodeJS 16 onwards (2021+, Node 16 recently went EOL in Sep 2023). The package-lock is also now only compatible with Node 16/npm 7 onwards.

Since this is technically a breaking change in npm/node compatibility, bumps the major version and updates the documentation as such.

Signed-off-by: Chad Wilson <chadw@thoughtworks.com>
@chadlwilson chadlwilson marked this pull request as ready for review September 26, 2023 02:52
@chadlwilson
Copy link
Contributor Author

Moved the patch-package to run before tests, which is "non-canonical" usage of patch-package, but since npm doesn't now seem to have a "only runs when dev dependencies are installed" hook and doesn't have built-in support for patching like Yarn 2+ does - this seems the best workaround for now, until/unless mock-fs is fixed and released upstream.

mock-fs seems to be unmaintained as there is a PR at tschaub/mock-fs#378 that would fix the issue on later Node versions without requiring one to change their production code in a way that avoids use of the more efficient "fast" code path for UTF-8 file reads.

@chadlwilson chadlwilson changed the title Require Node 16+ with build on Node 20 Require Node 16+ while building/testing on Node 20 Sep 26, 2023
haroon-sheikh
haroon-sheikh approved these changes Sep 26, 2023
View reviewed changes
Copy link

@haroon-sheikh haroon-sheikh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@sriv sriv merged commit 4c4a021 into master Sep 26, 2023
8 checks passed
@haroon-sheikh haroon-sheikh deleted the node-20 branch September 26, 2023 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@haroon-sheikh haroon-sheikh haroon-sheikh approved these changes

@zabil zabil Awaiting requested review from zabil

Assignees

@chadlwilson chadlwilson

Labels
ReleaseCandidate If a PR is tagged with this label, after merging it will be released technical
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@chadlwilson @haroon-sheikh @sriv