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

Bug: unsupported-features/node-builtins-modules doesn't work for semver range in v17 #250

Closed
1 task
teppeis opened this issue Apr 20, 2024 · 6 comments · Fixed by #252 or #257
Closed
1 task

Bug: unsupported-features/node-builtins-modules doesn't work for semver range in v17 #250

teppeis opened this issue Apr 20, 2024 · 6 comments · Fixed by #252 or #257
Labels

Comments

@teppeis
Copy link

teppeis commented Apr 20, 2024

Environment

Node version: v20.12.2
npm version: 10.5.0
ESLint version: 8.57.0
eslint-plugin-n version: 17.2.1
Operating System: macOS

What rule do you want to report?

unsupported-features/node-builtins-modules

Link to Minimal Reproducible Example

https://eslint-online-playground.netlify.app/#eNpdkl1PwyAUhv8K4Woma7cmxmnNduW9N8abMROkx6WzBTzAnNn63z2U7pMr+nJ4eDh0zx2qCexkaxvIN46XXBntPJPOAXo2Zwg/oUYYCZ4iwe+ehdbwO5TkCBtQ3o1izMc8B9fU2qMimtHE2wvNmOCw86ArJ3jJlvTVF5UIyrQt5VAJPqYq24R1rUs9uVpZjRPDSqQjX62vyTGSenZPV618B3S0EHPBG+nBkWy/M1Y4E1DB25+FVNCaKjQ0j+vdwEdKLrlpCK4n2mRBu2CtQQ9V9gXSBwRHeQXZZ6gbXyej5XknWSEaPDnEcQPeXiov5k/5NJ8mpTS643TVawrdUYetVN9yHV/rqr+aGndlT95kl9gfxX1ezPIpOxzYYl48nA863r2C7QvY2HCt6htOeq1Eerzoacqz9GjZcI1iNoDJlv6I7h9Tur5F

What did you expect to happen?

This works fine (expectedly fail),

"n/no-unsupported-features/node-builtins": ["error", {version: "9.0.0"}]

but this doesn't work (unexpectedly success) in v17: demo

"n/no-unsupported-features/node-builtins": ["error", {version: ">=9.0.0"}]

v16 works fine: demo

Participation

  • I am willing to submit a pull request for this issue.

Additional comments

Since specifying the version by range is common in the engines field of package.json, this bug is likely to occur in many cases.
However, this is not included in the test cases.

The logic of this range comparison seems to be the cause.

const range = getSemverRange(
[
...supported.map(
version => `>= ${version} < ${major(version) + 1}`
),
`> ${major(latest)}`,
].join("||")
)
if (range == null) {
return false
}
return configured.intersects(range)

@teppeis teppeis added the bug label Apr 20, 2024
@scagood
Copy link

scagood commented Apr 20, 2024

Thank you for this report!

We have two options here:

  1. Convert the range to a minimum version
  2. Throw an error to state that the config must not be a version range

I think in this context that converting it to a minimum version is the best course of action.

What do you think @aladdin-add?

@teppeis
Copy link
Author

teppeis commented Apr 21, 2024

  1. Throw an error to state that the config must not be a version range

What about the engines field of package.json? Mostly range is used there.

  1. Convert the range to a minimum version

When the engines field is >=18.10.0 || >= 20.0.0, it should be converted to 18.10.0 ? When supported versions are ["20.1.0", "18.9.0"], it cannot be determined whether it is supported using the minimum version.

It would be better to use the same logic as before.
https://github.com/eslint-community/eslint-plugin-n/blob/16.6.2/lib/util/check-unsupported-builtins.js#L54-L60

@scagood
Copy link

scagood commented Apr 21, 2024

The same section you highlighted can be found here on master:
https://github.com/eslint-community/eslint-plugin-n/blob/master/lib/util/check-unsupported-builtins.js#L40-L47

@teppeis
Copy link
Author

teppeis commented Apr 21, 2024

The difference:

  • v16: return !configured.intersects(unsupportedRange) (configured is expected to be range)
  • v17: return configured.intersects(supportedRange) (configured is expected to be only minimum version)

aladdin-add added a commit that referenced this issue Apr 24, 2024
fixes #250

Signed-off-by: 唯然 <weiran.zsd@outlook.com>
@aladdin-add
Copy link

yes, the configured version should be a subset of the supported range. I have made a fix: #252

@scagood
Copy link

scagood commented Apr 28, 2024

A fix was implemented in #252, but due to an issue in semver we needed to revert it.

For now I wonder if we can solve this another way 🤔

aladdin-add added a commit that referenced this issue Apr 30, 2024
…257)

* fix: unsupported-features/node-builtins-modules

#250

* Reapply "fix: unsupported-features/node-builtins-modules range compare" (#254)

This reverts commit caecf3e.

* Update lib/util/check-unsupported-builtins.js

Co-authored-by: Sebastian Good <2230835+scagood@users.noreply.github.com>

* chore: remove a unrelated test

---------

Co-authored-by: Sebastian Good <2230835+scagood@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants