Bring in a "require-*"/"no-*" kind of rule

[JoshuaKGoldberg]

https://npmpackagejsonlint.org/docs/rules#require-node-rules has a bunch of require-* rules for requiring properties to exist, such as require-author, require-bin.

This seems like it'd do well as a general-purpose ESLint rule. It could:

• By default: enforce the required properties that will prompt a warning if missing
• By configuration: be extended to any arbitrary property

Maybe, package-json/require-properties or package-json/require-package-properties as a name? The latter seems a little overly specific, so I'm leaning towards require-properties maybe?

But, what about https://npmpackagejsonlint.org/docs/rules/#disallowed-node-rules? Should we unify into one big rule for requiring which properties do or don't exist?

I also think that https://npmpackagejsonlint.org/docs/rules/required-node/require-scripts can be treated as a subset of https://npmpackagejsonlint.org/docs/rules/scripts/prefer-scripts for this rule. As in, this same rule can require both that scripts exists and what values it contains. Someone please yell at me if that's not ideal.

Blocked on #40, but once that PR is merged this will be good to go. ✅

[MrHBS]

If I am understanding correctly, this way I can lint against adding useless properties for private apps, like author, name, version etc. Is this correct? I am basically trying to come up with the smallest possible package.json scheme for an app.

[JoshuaKGoldberg]

Yup! You'd have to configure the list yourself, as what constitutes as "useless" changes depending on the context.

[michaelfaith]

Maybe one rule called restrict-properties? And it has an array for require and one for disallow? Something like that?

[JoshuaKGoldberg]

Coming back to the require-* kinds of rules, I don't think making a single catch-all require-properties rule is a good idea. Users tend to like to granularly configure things, and disable specific ones as needed.

Also relevant, JoshuaKGoldberg/package-json-validator#80: splitting up package-json-validator into granular APIs that can each be called by a single rule in this plugin.

I think that leans this package to a nice direction for this issue:

• Granular require-* rules for fields like name, version, etc.
• Granular valid-* rules for those fields too
  • I think these should be separate from require-* - you might have a collection of package.json files that need valid entries for some properties iff they exist
• A single disallow-properties rule for properties that must not exist
• A single require-properties rule for properties that must exist

For the *-properties rules I'm not very deadset that they shouldn't be unified into one restrict-properties.

cc @michaelfaith - thoughts?

[michaelfaith]

What criteria do we use for whether a property should have its own rule vs be configurable in the single umbrella rule? It feels a little odd to have some properties be broken out as dedicated rules while others are configured as part of one big rule. The importance of a property is such a relative thing, where to draw that line becomes challenging. I do agree that granular rules feels a bit more natural, but having some be granular and some not be could be confusing.

[JoshuaKGoldberg]

Agreed, having some be granular and some not would be confusing. Proposal: how about we put all properties in their own rules? That's my preference more and more.

[michaelfaith]

Proposal: how about we put all properties in their own rules? That's my preference more and more.

That sounds good to me. Then there's the other part of the question: should there be separate require- and validate- sets of rules. Personally, I think it makes sense to have those be different things. Taking #786 as an example, having format validation as part of a rule called require-license doesn't seem to line up. So maybe require- rules would just be "does this thing exist or not". In that case, would valid- check that the property exists and check that it's valid, or just checking that if it does exist, it's valid?

[JoshuaKGoldberg]

would valid- check...

I think the latter, just checking that if it does exist, it's valid. 👍 otherwise they duplicate the require- rules.

[michaelfaith]

I think that's right. I can definitely see a case where someone would want to use valid without require. But I guess checking my original gut feel: is there a case where someone would want to use require without valid? That could possibly make a case for require to do both after all, and valid is just a less strict version of require? Sorry, just thinking out loud. Bikeshedding 😅

[michaelfaith]

I've talked myself into agreeing that require should just be requiring, haha. How I ultimately got there: I'm not sure it'll be the case, but it's certainly possible that some of the valid rules are in some way opinionated. Take the exports property as an example. There's certainly a world in which exports could be validated in an opinionated way. For instance, flagging a default property, if there's already an import and require condition for the same path, since the combination of import and require cover all of the cases. But, at the same time, some people like to have the default there anyway as a form of future proofing. In the event that some day there's some new resolve condition in addition to import and resolve it's possible their package still works ok, because they have that default to fallback on. So, if we had a valid rule that did some form of validation that someone disagreed with, and we've bundled validation into the require rules, then they can't use that property's require rule either.

So yeah, tl;dr, I think we're both aligned on the following

• require- rules and valid- rules should be distinct and separate
• each property that the plugin supports will have its own set of rules (at least require-)

Does that sound right?

[JoshuaKGoldberg]

💯 🤝

We're in alignment. This is great! Looking forward to making progress on this.

[JoshuaKGoldberg]

OK! I split out a few dozen require- and valid- issues -795 through 843- based on the table in #42. There are almost certainly some missing from user error on my end and/or from package.json fields that were added after other plugins created their rules. I'm sure we'll discover at the latest when we split apart the valid-package-def rule.

Closing this issue out as resolved discussion. Cheers!