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

fix: proper version check in hasStableEsmImplementation #4653

Merged
merged 1 commit into from Jun 10, 2021
Merged

fix: proper version check in hasStableEsmImplementation #4653

merged 1 commit into from Jun 10, 2021

Conversation

alexander-fenster
Copy link
Contributor

Fixes #4652.

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions.

Description of the Change

The version check in hasStableEsmImplementation returns true for any version that has minor component >= 22 (e.g. for version 10.24.x which is currently the latest one in the 10.x branch). Even though Node.js v10 is officially not supported, I still see a lot of sense to fix this and make hasStableEsmImplementation work properly.

Alternate Designs

None

Why should this be in core?

A lot of CI tasks are still using 10.x for compatibility reasons.

Benefits

No unintended breaking change to Node.js v10.24.x users.

Possible Drawbacks

None known.

Applicable issues

#4652

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 94.396% when pulling b0e6bd6 on alexander-fenster:version-check into 8339c3d on mochajs:master.

@juergba juergba requested review from giltayar and juergba June 9, 2021 06:07
@juergba juergba added semver-patch implementation requires increase of "patch" version number; "bug fixes" area: usability concerning user experience or interface labels Jun 9, 2021
@juergba juergba added this to the next milestone Jun 9, 2021
@juergba juergba added the run-browser-test run browser tests on forked PR if code is safe label Jun 9, 2021
@github-actions github-actions bot removed the run-browser-test run browser tests on forked PR if code is safe label Jun 9, 2021
@giltayar
Copy link
Contributor

I would strongly recommend using Mocha v8 if you're using Node v10.

But, hey, why not accept this PR? 😊 LGTM.

But please note that accepting this PR should not mean that we support Node v10 in Mocha >= v9, as there may be other incompatibilities that we are not aware of.

@juergba juergba merged commit b20f3c9 into mochajs:master Jun 10, 2021
@juergba juergba added the area: node.js command-line-or-Node.js-specific label Jun 10, 2021
@alexander-fenster
Copy link
Contributor Author

Thank you folks! Re: "using Mocha v8 if you're using Node v10" - We don't really use Node v10 :) We just want to keep it in our CI for just a little longer, and don't want to pay for it by staying on the old version. Sure, if a real breaking change comes, we'll have no choice, but here in this case the trivial fix could make it live longer. So - again - thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: node.js command-line-or-Node.js-specific area: usability concerning user experience or interface semver-patch implementation requires increase of "patch" version number; "bug fixes"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stopped working on Node.js v10.24: "Error: Not supported" caused by ESM import
4 participants