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

Raise an error when the version of node is older than package.json's engines.node #1922

Merged
merged 8 commits into from
Feb 10, 2022

Conversation

mattwynne
Copy link
Member

@mattwynne mattwynne commented Feb 8, 2022

Description

Adds a warning if the process.version is too old for Cucumber to work.

Motivation & context

Fixes #1890 - we had a user who was running an older node version and got confused.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have read the CONTRIBUTING document.
  • My change needed additional tests
    • I have added tests to cover my changes.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

@mattwynne
Copy link
Member Author

It's nigh-on impossible to test this other than manually. Here's the result of my manual test:

➜  cucumber-js git:(check-node-version) node --version
v11.10.1
➜  cucumber-js git:(check-node-version) npm run feature-test
(node:57566) ExperimentalWarning: The fs.promises API is experimental

> @cucumber/cucumber@8.0.0-rc.2 prefeature-test
> npm run build-local


> @cucumber/cucumber@8.0.0-rc.2 build-local /Users/matt/git/github.com/cucumber/cucumber-js
> genversion --es6 src/version.ts && tsc --build tsconfig.node.json && shx cp src/importer.js lib/ && shx cp src/wrapper.mjs lib/


> @cucumber/cucumber@8.0.0-rc.2 feature-test
> node ./bin/cucumber-js

internal/modules/cjs/loader.js:615
    throw err;
    ^

Error: Cannot find module '/Users/matt/git/github.com/cucumber/cucumber-js/lib/check_node_version.js'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:613:15)
    at Function.Module._load (internal/modules/cjs/loader.js:539:25)
    at Module.require (internal/modules/cjs/loader.js:667:17)
    at require (internal/modules/cjs/helpers.js:20:18)
    at Object.<anonymous> (/Users/matt/git/github.com/cucumber/cucumber-js/lib/importer.js:1:63)
    at Module._compile (internal/modules/cjs/loader.js:738:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:749:10)
    at Module.load (internal/modules/cjs/loader.js:630:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:570:12)
    at Function.Module._load (internal/modules/cjs/loader.js:562:3)

@coveralls
Copy link

coveralls commented Feb 8, 2022

Coverage Status

Coverage decreased (-0.08%) to 98.128% when pulling 3e67e38 on check-node-version into c1ed743 on main.

@mattwynne
Copy link
Member Author

Two possible improvements:

  1. It might be nicer to print the error message without the stacktrace, but I'm not sure how to do that without adding console-specific code, which feels wrong in this file. (I'm kinda surprised there's not an error handler at the UI level?)

  2. It seems like putting the check in the importer is the right place to have it, since that's where we need to be on that version, but I guess it could move more centrally, and look up the right version from the package.json?

Copy link
Contributor

@davidjgoss davidjgoss left a comment

Choose a reason for hiding this comment

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

It might be nicer to print the error message without the stacktrace, but I'm not sure how to do that without adding console-specific code, which feels wrong in this file.

It seems like putting the check in the importer is the right place to have it, since that's where we need to be on that version, but I guess it could move more centrally

Lack of ESM support is the symptom you ran into, but I think there's a good chance of us using some other language feature or standard library that's only in our supported versions. I'd advocate for putting it here https://github.com/cucumber/cucumber-js/blob/main/src/cli/run.ts which is the entry point for the CLI, and where you could just do console.error and then process.exit(1) because as you've noted the stack trace isn't useful in this scenario.

...and look up the right version from the package.json?

Nice to have, but not essential IMO. I think it would be sufficient to add a section to the CONTRIBUTING.md on how to manage dropping an EOL'd Node version - up to you though!

src/importer.js Outdated Show resolved Hide resolved
@mattwynne
Copy link
Member Author

OK this is good enough now IMO. Please check and merge!

@mattwynne mattwynne changed the title Raise an error when the version of node is older than v12 Raise an error when the version of node is older than package.json's engines.node Feb 9, 2022
@mattwynne mattwynne merged commit 5295a45 into main Feb 10, 2022
@mattwynne mattwynne deleted the check-node-version branch February 10, 2022 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warn users on old (pre-ESM) node versions
5 participants