ESLint Plugin: Add rule dependency-group

[aduth]

This pull request seeks to introduce a new ESLint rule to enforce expectations around adherence to the dependencies grouping conventions as outlined in the Coding Guidelines.

Specifically, it enforces:

• An import is preceded by "External dependencies", "WordPress dependencies", or "Internal dependencies" as appropriate by the import source.

This is meant to be quite minimal in its current form, as there are a shocking number of inconsistencies in the codebase.

To make review somewhat easier, current (assumed temporary) tolerances include:

• Normalize /** and /*
  • Aside: I think we need to make a decision here, and as much as it'll be a pain in that it requires changes to quite nearly every file in the codebase, my leaning is toward /* as these are not JSDoc
• Case insensitive "Dependencies" vs. "dependencies"
• Ending period
• "Node" dependencies as an alias for External

Future improvements will include:

• Guaranteed order "External", "WordPress", "Internal"
• Enforce expected (and only expected) newline placement around and between import statements

It may help simplify review to separately review individual commits.

Testing instructions:

While these changes touch many application files, it should not be in a way which impacts runtime behavior, as they are merely shuffling of imports and revisions to docblock comments.

Ensure tests pass:

npm test

[gziolo]

This is one of the most needed ESlint rules given that we enforce those patterns in code reviews. It should make it so much easier to review PRs of new contributors as tools will help us to catch it early one and easily reference the issues based on reports on Travis.

I quickly scanned changes introduced and it all looks great. I will have a closer look at the actual logic of the rule later today.

At the same time, I hope we don't make it harder to start to contribute by being so strict about so many things. It's hard to judge when you are already familiar with all the conventions that WordPress and Gutenberg on its own enforce on the codebase. As long as the messages printed from the ESLint are easy to follow, it should be fine.

[talldan]

This is looking good, I tested it out both in AST Explorer and my editor, and I think would be valuable to merge in its current state.

I noticed it is possible to trick the rule if you really mess up the imports, but I'd hope that would be caught in a code review:

/**
 * External dependencies
 */
/**
 * WordPress dependencies
 */
import { autop } from '@wordpress/autop';
import { applyFilters } from '@wordpress/hooks';
import { parse as defaultParse } from '@wordpress/block-serialization-default-parser';
import { parse as hpqParse } from 'hpq';
import { flow, castArray, mapValues, omit, stubFalse } from 'lodash';

Creating a fix function for the rule would be interesting. I suppose the code would have to gather all the imports, categorise them, and then replace the entire lot. On second thoughts, maybe not!

[oandregal]

Perhaps we could use incorrect/correct instead of warning here, so as to prevent confusion with the error/warning setting of the rule?

Suggested change

	The following patterns are considered warnings:
	The following patterns are considered **incorrect** code:

[aduth]

I made it this way thinking I was adhering to the format set forth by ESLint, but I guess at some point they must have changed from a pattern of:

The following patterns are considered problems:

To:

Examples of incorrect code for this rule:

eslint/eslint@8c29946#diff-61fab3ca0b432c4678c6546a292fe58f

I can update accordingly.

[oandregal]

Suggested change

	The following patterns are not considered warnings:
	The following patterns are considered **correct** code:

[gziolo]

I noticed it is possible to trick the rule if you really mess up the imports, but I'd hope that would be caught in a code review

We can tweak that if it ever happens unless it's an easy update which verifies if a given comment is followed with code import :)

[gziolo]

Great work, I was waiting for this rule for so long 🚢 🚀

[gziolo]

Future improvements will include:

• Guaranteed order "External", "WordPress", "Internal"
• Enforce expected (and only expected) newline placement around and between import statements

eslint --fix first maybe? :)

[aduth]

At the same time, I hope we don't make it harder to start to contribute by being so strict about so many things. It's hard to judge when you are already familiar with all the conventions that WordPress and Gutenberg on its own enforce on the codebase. As long as the messages printed from the ESLint are easy to follow, it should be fine.

Yeah, I definitely agree. I don't know that it's always been supported, and I'm sure it may not be leveraged in many (most? all?) IDE integrations, but apparently there's an option to provide a documentation URL with the rule:

https://eslint.org/docs/developer-guide/working-with-rules

I'll look to add this.

Generally, I see how this could be problematic for someone who's not familiar with the conventions to encounter some rule which tells them to "Add a dependency docblock", as it's not immediately clear what the resolution action is.

As mentioned, a fixer here could help as well. I'm not sure how possible it is to implement, considering that comments are treated very particularly by parsers (i.e. they're not Nodes in the AST, they're provided separately via context.getSourceCode().getAllComments()). I'll look into it, but I also don't think it need block us here.

Finally, we should always be open to considering the value of any enforced convention, and weigh it against the overall cost (time, willingness) in conforming to it. In this instance, I'd still argue in favor of it, as I think there's value in providing some arrangement to imports, though the specifics of that arrangement aren't too important. At this point, it's more just that we've landed at this convention, and it's important we implement it consistently (which we're very clearly not, prior to this pull request).

[aduth]

I noticed it is possible to trick the rule if you really mess up the imports, but I'd hope that would be caught in a code review:

Yeah, and honestly I wouldn't doubt that there'd be some remaining violations in the code which aren't too different than the example you provide. I'd hope to refine and tighten the restriction iteratively. For these, I could imagine it'd fall as part of an implementation to enforce the specific order of comment blocks.

[aduth]

The rebased branch reflects the following changes:

• Rename rule from dependencies-docblocks to dependency-group
  • "DocBlock" adds an unintentional connotation that these would take the /** form. The new name also reflects the intent of the rule over its implementation.
• Rephrase "incorrect code" in documentation (ESLint Plugin: Add rule dependency-group #13757 (comment))
• Add more comments to the rule implementation
• Add documentation references to the rule meta
• Implement a fixer 🎉 (ce8af4f)