search

tclindner / npm-package-json-lint

Configurable linter for package.json files
https://npmpackagejsonlint.org
MIT License
223 stars 33 forks source link

move checking to `engines-type` #203

Open piranna opened 4 years ago

piranna commented 4 years ago

https://github.com/tclindner/npm-package-json-lint/blob/a4d6e7cfcfd587ea0646ab01a5bde64634c138db/src/rules/valid-values-engines.js#L24-L35

valid-engines-values is also checking that the values are of the correct type, instead of just only cheking they are one of the specified versions. I think this type checking should be moved to engines-type to ensure it's a mapping object of strings instead of just only a plain object, and at the same, this one should do the checking against semver ranges instead of just only checking that specified versions match some fixed ones.

tclindner commented 4 years ago

Hi @piranna - thanks for the issue! I have a handful of questions:

  1. Is there any harm in checking the type here so we can provide more information in the version range is invalid?
  2. This rule follows the format of other valid-values-* rules. They check that the value specified matches the config. This is really useful for shared config modules that are used across many repos and/or monorepos.
  3. I might be missing understanding, but are you saying that valid-values-engines is not validating the semver ranges?
  4. If you don't find the functionality described in 2 (above) useful, we could create a new rule that aligns with the format rules. The new rule, engines-format could focus on validating the semver ranges of the engines object. Is that what you are looking for?
piranna commented 4 years ago
  1. Is there any harm in checking the type here so we can provide more information in the version range is invalid?

Not at all, but the problem is that it's a misnomer. Rule check for valid values, but it's checking if it's a valid range regarding to semver format, not a range compatible with a specified version.

  1. This rule follows the format of other valid-values-* rules. They check that the value specified matches the config. This is really useful for shared config modules that are used across many repos and/or monorepos.

That's the point, there's no config :-) I would have expected similar to the behaviour or other similar rules than it would validates than engines version range matches to a provided version.

  1. I might be missing understanding, but are you saying that valid-values-engines is not validating the semver ranges?

Yes, that's it, what's doing semver.validRange(versionRange) === null is just checking that versionRange is a valid semver range according to semver format, it's say, that's of valid type. It's not checking for its actual value.

  1. If you don't find the functionality described in 2 (above) useful, we could create a new rule that aligns with the format rules. The new rule, engines-format could focus on validating the semver ranges of the engines object. Is that what you are looking for?

Yes, that would be nice, I think it's the current behaviour of this rule. It's similar to version-format, and in fact it's calling to semver.valid()