move checking to engines-type

[piranna]

npm-package-json-lint/src/rules/valid-values-engines.js

Lines 24 to 35 in a4d6e7c

	for (const engineDefinition in packageJsonData[nodeName]) {
	const versionRange = packageJsonData[nodeName][engineDefinition];
	if (semver.validRange(versionRange) === null) {
	return new LintIssue(
	lintId,
	severity,
	nodeName,
	`engines, ${engineDefinition} version range is invalid. Currently set to ${versionRange}`
	);
	}
	}

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]

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]

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.

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.

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.

3. 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.

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?

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()