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

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

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.

…h in Node 20

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

Signed-off-by: Chad Wilson <[email protected]>
@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.

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 <[email protected]>
@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
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
Labels
ReleaseCandidate If a PR is tagged with this label, after merging it will be released technical
Development

Successfully merging this pull request may close these issues.

3 participants