Add WordPress JSDoc ESLint configuration

[atimmer]

Description

The WordPress ESLint configuration plugin has a JSDoc configuration. I've added this to the ESLint configuration to see how much would fail. And start the discussion about how to integrate this.

Current stats:

✖ 1504 problems (869 errors, 635 warnings)

[youknowriad]

Maybe we should enable this as a "warning" to start fixing those as we add/update code?

[atimmer]

This is ready to be reviewed.

[youknowriad]

Why this rule is different?

[ntwb]

Via https://www.npmjs.com/package/eslines#how-to-use-it:

"The no-unused-vars rule won't be downgraded, though - this is one case where changing one line can cause a linting error in a different one so we recommend preventing it from downgrading."

[youknowriad]

This is working pretty well for me. looking forward getting rid of all these warnings.

[ntwb]

In e352c97 I updated to a more recent commit of eslint-plugin-wordpress, there were no JSDoc rules changes between this and the previous version used, just this version is the latest is all

[ntwb]

This is great, thanks @atimmer 👍

[noisysocks]

This implementation shows errors for lines that differ from origin/master and warnings for everything else. This means that, in a PR, Travis CI will fail until jsdoc comments are added to any methods that that PR touches.

Is that intentional? I read @youknowriad's comment as that we only ever want warnings.

[atimmer]

@noisysocks That comment was before eslines was introduced. The idea was always for these to be errors, but it is hard to enable this for the entire codebase without first cleaning up everything. eslines solves this problem. It is basically the boy-scout rule in a tool.

[noisysocks]

Cool, just thought I'd check since it wasn't explicitly mentioned in the PR description! 😄

[mcsf]

I'm not convinced this rule suits the project well, at least in its current form. Even with eslines, it requires a lot of attention, arguably too much. Examples:

• Functional components, incl. small ones. The JSDoc will be a repetition of what is already obvious in the code:

/**
 * Returns a menu of tools representing actions on the editor's content.
 *
 * @returns {WPElement} Element.
 */
export default function EditorActions() {
  return (
    <MenuItemsGroup className="editor-actions" label={ __( 'Tools' ) }>
      <CopyContentButton />
    </MenuItemsGroup>
  );
}

Assuming the component takes props, that will require an additional * @param {Object} props React props passed to the component., though the tool can't require that every possible prop be described.

Large or otherwise significant or hard-to-grasp components will obviously warrant JSDoc, but one of the strengths of our compositional paradigm is that it encourages breaking down functionality in layers and spawning small components. If Foo renders a small collection of Bars, it's preferable to add a function Bar() {} to use in a Array#map than to inline everything in Foo#render or to delegate to Foo#renderBars. require-jsdoc works against this.

• Component methods. Case in point:

CopyContentButton is a small component, as it's basically a wrapper for ClipboardButton with some local state and some Redux state relaying. Of the four methods that the class comprises, none of them should even have a JSDoc—let alone be required one. Two of the methods—constructor and render—are part of the React standard, and the two remaining ones are state-setting one-liners.

---

I think the main point of this is that, whenever we're talking about components, we're dealing with a standard interface:

• Props go in, element goes out.
• Props can be optional, they can have default values, and this should be visible in the function signature or in the render method's const { foo, bar = 'BAR' } = this.props; assignment anyway.
• Methods tend to fit one of a small set of types: lifecycle methods (incl. constructor and render), state changers or similar procedures (e.g. callers of bound action creators), or other tiny helpers. Most of the time these don't need comments—IMO, they detract from the code and add a bit of inertia. For any other method, comments should be considered as a general rule—something that, granted, isn't easy to codify in a linter.

With that premise that it is a standard interface, it follows that it's wasteful to document said interface every time it's implemented.

A trivial but potentially naïve way forward would be to give components a free pass by excluding any function declaration whose identifier has an uppercase letter as the leading character. For class components, perhaps exclude classes defining a render method.

[youknowriad]

👍 to @mcsf concerns, I've already shared similar concerns in #core-js meetings but now facing them in reality shows that it's really too much for a standard interface.

[jdevalk]

This is simply laziness, I'm sorry. It's obvious what is in the code if you're well versed in the code, if you're not, this:

<MenuItemsGroup className="editor-actions" label={ __( 'Tools' ) }>
      <CopyContentButton />
</MenuItemsGroup>

Is not necessarily self-explanatory. Spending two to three seconds to write that it returns a copy content button really does help.

[afercia]

In a large open source project like WordPress, good documentation is key. Maybe in smaller projects or in projects internal to a company, the documentation effort may have a different priority and may adapt to specific momentary needs. In WordPress, I'd strongly suggest to not repeat the same mistakes done in the past and document everything.

I'd suggest to try to not think as an expert developer looking at code that was coded now by yourself or your colleagues. Instead, I'd recommend to make an effort to think at someone with a low level of expertise who, in 2 years from now, reads for the first time your code. Expecting that they understand what is obvious for you is just an assumption.

[mcsf]

In a large open source project like WordPress, good documentation is key.

I wholeheartedly agree, but in order for that argument to be useful here, it needs to be established that adding JSDoc to the entities at hand would constitute adding good documentation, which is the subject of debate. Otherwise, the implication would've been that I'd argued against good documentation.

Expecting that they understand what is obvious for you is just an assumption.

I also don't refute this. I expect that I and others should be called out on whenever we push code that's hard to grasp as blind spots develop from our experience with the codebase.

document everything

IMO, if a codebase needs to be entirely documented with natural language, that's symptomatic of a pretty unhealthy project. The key in that sentence is natural language; code should act as documentation. An easy instance of this is function signatures, even in a untyped language like JS. JSDoc absolutely should fill the gaps, but aiming to fill more than the gaps means we have information duplication. That duplication entails some added cost of maintenance, as well, arguably, as some overhead for the reader of the codebase. Circling back to @jdevalk's highlight:

It's obvious what is in the code if you're well versed in the code, if you're not, this:

<MenuItemsGroup className="editor-actions" label={ __( 'Tools' ) }>
      <CopyContentButton />
</MenuItemsGroup>

Is not necessarily self-explanatory.

We mustn't omit the function signature, as it is vital:

export default function EditorActions() {
  return (

EditorActions is a fragment of UI. It doesn't depend on parent props (or any other kind thereof, e.g. Redux connection), and returns an arrangement of other fragments or units or UI. In this particular case, I think a useful doc string would've been something like "Returns a menu of tools that represent actions to be performed on the editor's whole content". That's the intent with which the component is created; from then on, EditorActions is its own thing, regardless of where it is used or what it contains. However,

Spending two to three seconds to write that it returns a copy content button really does help.

That I disagree with, because the specifics of what it contains shouldn't have to bleed into the documenting description.

1. If the menu changes to accommodate other tools, does the JSDoc have to keep growing to enumerate what is already a code-based enumeration? The natural-language doc should provide the introduction that makes reading the code straightforward (cf. my suggested JSDoc), and not perform a word-per-word translation of the code. Furthermore, it is expected that EditorActions will be a hookable entity pretty soon, meaning just about anything could be added to it; I bring this up because this is a very common scenario in the WP code tradition. A description that enumerates is useless in that context, a statement of intent isn't.
2. Many fragments of UI are by design reusable, and others yet see their intents change over time. This means that a JSDoc along the lines of "this menu is part of the ellipsis menu […]" would be a form of entanglement—not code-borne, but via the contract of natural language. And, though a developer can change the JSDoc over time, compilers aren't aware of the contract there, and can't complain about inconsistencies.

This is simply laziness, I'm sorry.

At the personal level, it's not my place to argue against this claim. However, having made my point above, I posit that it can actually be counterproductive to port everything to comments. Duplicating is easy; a bit of editor fu and a quick sentence do the trick. I'm more concerned with the maintenance overhead and, paradoxically, the reading overhead.

Finally, I should probably reiterate that I don't think JSDoc to be generally useless. Not at all. But there are trade-offs to globally enforcing it.

[mcsf]

Aside, what are your thoughts on method-level enforcement?

[noisysocks]

I think that the component model used in modern JavaScript apps negates a lot of the need for stringent inline documentation. The strength of this model is that a component is isolated from the rest of the application: a mortal developer is realistically able to visualise the inputs, state and output of a single component.

My worry with this rule is that it comes at the expense of discouraging developers from changing how a component works internally. In my experience, a constantly changing codebase is a healthy codebase.

My preference is that we require JSDoc comments on only the contract that a module or component has with its outside world. That is:

• Any functions, variables, classes or constants exported using export
• Any props that a React component accepts

[ellatrix]

Just opened #4504, before I was aware of this conversation. Please let's remove at least the requirement for JSDocs on on React component methods. 🙂

[gziolo]

My preference is that we require JSDoc comments on only the contract that a module or component has with its outside world. That is:

• Any functions, variables, classes or constants exported using export
• Any props that a React component accepts

@noisysocks, this sounds like a good default for Eslint verification. It would still allow to auto-generate docs for all code that is consumed by contributors in most of the cases.

@mtias I'm wondering how this new Eslint rules will impact README files. They were often used to present API methods, which duplicates what JSDoc does. Should we standardize that and create scripts to generate those sections? Actually, this is how things are maintained in WP-CLI. @schlessera or @danielbachhuber can give us more details on that.

[ntwb]

I've now partially reverted this PR in #4510, no for more JSDOc warnings from ESLint for now.

Some eyes on #4506 would be great, it's a require-jsdoc drop in ESLint rule that allows us to not require JSDocs on methods 😄

[ntwb]

@aduth Fixed most instances (I hope) of these in #4563

[gziolo]

@ntwb, we can remove eslines, too. It was only necessary because we had all those rules set as warnings. What eslines does is it promotes warnings to errors for the modified lines.

[aduth]

In case there are others like me using the DocBlockr Sublime Text plugin, here's a user configuration which can be added to adhere to the updated documentation styling:

Preferences > Package Settings > DocBlockr > Settings - User

{
  "jsdocs_return_tag": "@returns",
  "jsdocs_spacer_between_sections": true,
  "jsdocs_per_section_indent": true,
  "jsdocs_lower_case_primitives": true
}

[ellatrix]

Thanks @aduth!

[schlessera]

Re. generated documentation like referenced above, you generally do this for everything that can be considered an "interface". So, you have the public interface, which is what external developers will use to create third-party blocks or integrations, and then you have the internal interface which deals with the intra-communication between your individual components/modules. Ideally, both of these categories should be documented in the code without fail and the external documentation should be generated out of this source code. This is the only reliable way of keeping the documentation in sync with the actual code.

As to the usefulness of the docblocks for simplistic methods...

You would not only add a short description of what and why (the why normally being something that is not obvious from the code alone), but also historical information that can inform future decisions... Knowing when something was introduced will help with debugging version-dependent bugs and be smart about deprecations. Knowing what discourse led to the current implementation avoids switching between two good-enough approaches every few months. There is a lot of potential information that you can put into such a docblock that will not necessarily help you understand the one or two lines of code, but help you make better decisions two years down the road.