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: ensure "version" always has a valid value in parseOptions #109

Conversation

ZauberNerd
Copy link
Contributor

The rule no-unsupported-features would sometimes fail with a TypeError:

Error while loading rule 'node/no-unsupported-features': Cannot read property 'split' of null
TypeError: Error while loading rule 'node/no-unsupported-features': Cannot read property 'split' of null
    at new Range (.../node_modules/semver/semver.js:780:20)
    at Function.intersects (.../node_modules/semver/semver.js:1305:8)
    at Object.freeze.features.Object.freeze.OPTIONS.reduce (.../node_modules/eslint-plugin-node/lib/rules/no-unsupported-features.js:191:44)
    at Array.reduce (<anonymous>)
    at parseOptions (.../node_modules/eslint-plugin-node/lib/rules/no-unsupported-features.js:175:41)
    at Object.create (.../node_modules/eslint-plugin-node/lib/rules/no-unsupported-features.js:281:25)

This happens, when neither "engines" are set in the "package.json",
nor "version" is set in the rule configuration but the rule still has
configuration options, because version will then be set to
options.version, even if that is undefined.

This commit ensures that version always has a valid value before
trying to construct a semver range from it.

@codecov-io
Copy link

codecov-io commented Feb 13, 2018

Codecov Report

Merging #109 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #109   +/-   ##
=======================================
  Coverage   98.21%   98.21%           
=======================================
  Files          43       43           
  Lines        1065     1065           
=======================================
  Hits         1046     1046           
  Misses         19       19
Impacted Files Coverage Δ
lib/rules/no-unsupported-features.js 97.39% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bbf4b60...25aac0a. Read the comment docs.

@mysticatea
Copy link
Owner

Oh, thank you for the contribution!

It looks a regression in #99 I had overlooked.
Would you move the change to getDefaultVersion function? The getDefaultVersion function should return a string always, but now it can return null.

The rule no-unsupported-features would sometimes fail with a TypeError:
```
Error while loading rule 'node/no-unsupported-features': Cannot read property 'split' of null
TypeError: Error while loading rule 'node/no-unsupported-features': Cannot read property 'split' of null
    at new Range (.../node_modules/semver/semver.js:780:20)
    at Function.intersects (.../node_modules/semver/semver.js:1305:8)
    at Object.freeze.features.Object.freeze.OPTIONS.reduce (.../node_modules/eslint-plugin-node/lib/rules/no-unsupported-features.js:191:44)
    at Array.reduce (<anonymous>)
    at parseOptions (.../node_modules/eslint-plugin-node/lib/rules/no-unsupported-features.js:175:41)
    at Object.create (.../node_modules/eslint-plugin-node/lib/rules/no-unsupported-features.js:281:25)
```

This happens, when neither "engines" are set in the "package.json",
nor "version" is set in the rule configuration but the rule still has
configuration options, because version will then be set to
`options.version`, even if that is `undefined`.

This commit ensures that `version` always has a valid value before
trying to construct a semver range from it.
@ZauberNerd ZauberNerd force-pushed the ensure-version-no-unsupported-features branch from e995a59 to 25aac0a Compare February 14, 2018 09:28
@ZauberNerd
Copy link
Contributor Author

@mysticatea updated, please take another look :)

@ZauberNerd
Copy link
Contributor Author

Hi @mysticatea just wanted to ask whether you had time already to take another look at this and if there's anything I can help with to get the fix released.

Copy link
Owner

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

I apologize for my delay. LGTM!

@mysticatea mysticatea merged commit 234703c into mysticatea:master Feb 23, 2018
@ZauberNerd
Copy link
Contributor Author

No worries, thank you!

@ZauberNerd ZauberNerd deleted the ensure-version-no-unsupported-features branch February 23, 2018 08:41
mysticatea added a commit that referenced this pull request Feb 23, 2018
ota-meshi pushed a commit to ota-meshi/eslint-plugin-node that referenced this pull request Sep 25, 2023
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.

None yet

3 participants