Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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 :)

# for free to join this conversation on GitHub. Already have an account? # 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