Eslint: Exempt TS files from jsdoc/require-param
#39458
Conversation
|
Size Change: +486 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
|
Just playing devil's advocate here — Even if the type of a parameter is defined in TypeScript, the Would removing the |
Not really, in the sense that our jsdoc lint rules are progressively applied. This rule only requires that every parameter is merely listed, and even that is only when someone chooses to add a JSDoc for the function in the first place. Something like It may act as a prompt for the person to add a param description, but in TS files I think it's a bad place to prompt it because we should encourage people to put param descriptions inline rather than in the JSDocs for the function. // Better ✨
type Props = {
/** My foo */
foo: boolean;
}I'm fine with scoping the override to the wp-components package only, since other packages might have different needs. |
There was a problem hiding this comment.
Something like
@param foois enough to satisfy it — no type or description required.
...
It may act as a prompt for the person to add a param description, but in TS files I think it's a bad place to prompt it because we should encourage people to put param descriptions inline rather than in the JSDocs for the function.
Good observations! I'd personally be ok with the changes from this PR, although I wonder if it's better to wait for @gziolo 's thoughts too.
I'm fine with scoping the override to the wp-components package only
I don't have a strong option here, it's probably fine to apply them to all TS files.
Yes, that's one thing. The other aspect is the tooling we currently use that extracts params from JSDoc to include them in the auto-generated documentation. @dmsnell and @adamziel, how do you handle the cases where you keep full JSDoc comments for functions but you omit types as they are already encoded in TypeScript? How does it work with the tool that generates API documentation? |
Just curious since I'm still rather new to the mix of JSDoc and TypeScript, but why should we encourage inline descriptions? In my limited experience I feel like both constructions have presented approximately the same outcome and experience, and personally I don't mind having the listing of params in the docblock, especially if the descriptions read more like sentences with multiple lines. /**
* @param name Name of the given steel to process; this
* could be one of several common names or
* the AISI-SAE specification code.
* @param cycles How many annealing rounds through which
* to run the steel. If omitted will use the
* recommendation for the given steel type.
*/vs. const process = (
/**
* Name of the given steel to process; this
* could be one of several common names or
* the AISI-SAE specification code.
*/
name: string,
/**
* How many annealing rounds through which
* to run the steel. If omitted will use the
* recommendation for the given steel type.
*/
cycles?: number
) => {
…
}Not a big deal, but even in that example I found the inline comments a bit more tedious for formatting and changing the formatting when the comment changes.
@sarayourfriend and I removed the requirement that JSDoc params list a type. For one, it was redundant with typed parameters; for two, after a quick survey and some consideration, we found that the types in the docs are generally not that helpful. Most are It's my strong opinion that JSDocs should intentionally not include typing information when TypeScript provides the types because JSDoc is manual and unconstrained while TypeScript is automated and verified. That being said I find a lot of value in having explanatory descriptions available for function arguments/parameters and so I find some kind of documentation there crucial, especially in a place like Gutenberg where we often have confusingly-named or outright inaccurately/misleadingly-named arguments. I'm in favor of keeping the requirement that params have documentation. Our tooling won't discover the inline comments but we've discussed deficiencies in our tooling and a desire to eventually move towards more TypeScript-native tooling which parses the code and extracts all this and provides links to type definitions and the like. My opinion boils down to a more general thought about listing: do people want to require documentation for function arguments? If so then leave this rule in unless we can verify the inline comments. If not then take it out; tinting is really really slow and can be quite annoying when it's wrong (because it assumes it's never wrong, even when presented cases such as the motivation for this very PR). |
There was a problem hiding this comment.
TSDoc, the TypeScript specific flavor of JSDoc, still requires params to be documented almost exactly the same way as JSDoc, just without the type. You're correct the type information is redundant but it doesn't mean we can change the way we document parameters entirely, especially not if we want to preserve existing developer experience for easy access to docstrings on function and constant declarations. Inline documentation isn't parsed the same way by babel or other AST builders and therefore isn't used by editors the same way for completion suggestions and inline documentation.
Here is a TypeScript playground that illustrates the difference for editors
Here are screenshots of each function in the playground being used to illustrate the difference in experience between the two:
Inline documentation as proposed by this PR
Standard TSDoc/JSDoc approach
If anything, I would suggest enabling the jsdoc linting rule that requires parameter descriptions for typescript files: https://github.com/gajus/eslint-plugin-jsdoc#user-content-eslint-plugin-jsdoc-rules-require-param-description
Additionally, enabling the TSDoc linter could also be a good idea as there are specific TSDoc conventions that can be followed (and could allow for automatic documentation extraction tools like APIExtractor in the future).
|
The example given in the PR description is indeed annoying and I don't have a good solution for it other than suggesting that adding |
Confirmed with the same behavior in PhpStorm |
Good question, I should've noted the reasoning. One advantage which I think is universally true, is that in an inline JSDoc we can use additional JSDoc tags like type Props = {
/**
* My foo prop.
*
* @default true
*/
foo: boolean;
}Another reason, which may or may not be relevant outside of wp-components, is how it interacts with the actual docgen. From our investigation for wp-components, we found that the only viable docgen for our requirements ( In any case, in trying out different docgens I realized that there's a substantial difference in the docgen requirements for React components vs. normal functions, so it might be the case that there is no config that's optimal for both contexts. |
The inline comment needs to be a doccomment ( function inlineDocs(
/** `p1` is for indicating when there are llamas in town */
p1: boolean
) {
return p1
} |
|
Ok, I tested with I limited the scope of the override to wp-components only (4deb885), and What do you all think? Still worth keeping in wp-components? |
|
@mirka oh sorry, I misunderstood the way inline docs work. They actually are better in some ways than the JSDoc style! You get the parameter documentation per-argument as you're passing it rather than only on the full function. Interesting. I don't know if the problem with You wouldn't need to use |
Can you share more about this? It sounds like you are trying to avoid this… /** Does something
* @param {Object} props
* @param {string} props.name name of thing
* @param {number} props.count times to do thing
* @param {boolean} props.flamp whether or not to flamp it too
*/
const Component = ( props ) => { … }Which, if that's the case, it might be a decent opportunity to create a separate type for the props anyway rather than couple them so tightly to the function's type. /** @typedef Props
* @property {string} name name of thing
* @property {number} count times to do thing
* @property {boolean} flamp whether or not to flamp it too
*/
/** Some component
* @param {Props} props component props
*/
const Component = props => { … }
const OtherComponent = ( /** @type Props */ props ) => { … }
I'm not aware of how to add examples for per-property values of an object such as with component props, but JSDoc does have a default value syntax we can use if we want. /** @param {number} [minimum=56] threshold at which process starts */This also works with all cases where we define types, such as in There's another option that I think may work for you too without giving up what you want and without changing linter rules. It's possible to go ahead and create TypeScript types for a component written in JS and import those types into JSDoc. This has the advantage that we get the flexibility of TypeScript and when we eventually convert over, we won't have to rewrite the JSDoc types as TS types. // some JS-only package, without a tsconfig
// types.ts
export interface Props {
/**
* Minimum threshold at which process begins
*
* @example use a negative number to invert the universe
* @default 56
*/
minimum?: number;
}// some JS-only package, in same directory as above
// index.js
/** @typedef {import("./types").Props} MeterProps */
/**
* Meter with minimum-threshold alarm.
*
* @param {MeterProps} props
*/
export default ( props ) => { … }I tested this out and here's what VSCode and PhpStorm are showing me. |
|
@dmsnell That's already how things are done in the components package (at least for newer components or older ones that have had type-checking added since the pattern was established by @ciampo and myself last year), specifically using a I think the idea is that annotating the |
Ah this is nice, thanks! I don't think I knew that syntax. Unfortunately react-docgen-typescript disregards JSDoc
Nothing big, this was just about how the linter demands a different number of params depending on where the props are destructured: /**
* @param props - Linter needs all the sub-params listed
* @param props.one
* @param props.two
* @param props.three
*/
const Component = ( { one, two, three }: Props ) => { … }versus /**
* @param props - Linter only needs the props obj param
*/
const Component = ( props: Props ) => {
const { one, two, three } = props;
} |
Right, which means I think we were talking about the same thing. Strangely this only impacts /**
* @param {Props} props - Linter only needs the props obj param
*/
const Component = ( { one, two, three } ) => { … }Seems like more mess from the combating opinions of tools that were all overly-simplistic to begin with.
Yes exactly, and this is my experience with docgen and JS linting tools in general. Again, for one reason or another, they simultaneously ignore important and valid uses of the language to fit the limited model they understand while imposing harsh restrictions on those other uses (instead of being more lenient). I guess we use Right now we made the change that the linting rule isn't important for all modules in the components tree, whether or not it's just React props or in any way related to React. That seems like a fairly broad quality escape-hatch for a small convenience. How important are those param descriptions? I don't know. How inconvenient overall is writing them out for the props? I don't know. This just feels neither here nor there, somewhere in between. It would seem better if we could somehow limit this exemption to React component props, but I don't think we have a way to do that or ascertain what a given argument is. |
Correct. We're basically trying to leverage TS data to build out interactive documentation for our component library.
I'm not too concerned about this at the moment for wp-components, because any non-trivial utility functions tend to be extracted out to But I do share your concern, and @ciampo and I will be on high alert for any ill effects that we see from this change. The good thing is that it's largely autofixable if we choose to revert. Personally I feel that human code reviewers tend to be better safeguards for insufficient/bad docs, and enforcing 100% coverage can sometimes even deter people from adding partial yet sufficient docs, or instead add meaningless noise ( The bulk of the benefit that |
A cursory glance turned up 32 functions in
this is really concerning 😆 and I think it's reason enough to take this opportunity to review the tooling entirely. we made the decision recently to stop requiring the types on those parameters project-wide so that people could more naturally slide into TypeScript. also it seems like the problem is again the tools are defective. I'm puzzled also that we added a second docgen library when we already consume one (not that the one in use by our custom docs-gen is better, it isn't). this just makes knowing the rules hard for everyone, particularly because those tools enforce contradictory styles. React components aren't the only files butting up against this small inconvenience either. I see it every day in all sorts of files I work in. Are React components really that different? |
The chain of dependency that inevitably leads to
Given that we don't have the appetite to build our own Storybook-equivalent from scratch, it makes sense to configure ourselves in a way that plays nice with Other wp packages have different documentation needs, so there might not be a one-size-fits-all. In an ideal scenario we'd have lint rules that look at JSDoc and inline types as a whole. Defective tooling, indeed.
Here's a concrete example of the kind of mess that can happen if we try to "work around" /**
* `BaseControl` is a component used to generate labels and help text for components handling user inputs.
*
* @param {WrongProps} props
*/
export const BaseControl: FunctionComponent< BaseControlProps > & {
VisualLabel: typeof VisualLabel;
} = ( { one, two, three } ) => {}
I guess in React you hit this problem a lot more frequently because of the prevalence of object destructuring. |
|
For the record, I agree with your frustrations here and am not trying to argue that.
This is where I think we're not on the same page. I'm not trying to say you should add all these things; I'm saying I think we can do better on this in a way that benefits everyone, or disables a faulty tool. Looking at the documentation for
I played around with some variations of these and it looks promising. The worst part about the config options is that the ones we want don't exist for Maybe we could discuss relaxing the requirement that destructured properties all get a |
* Eslint: Exempt TS files from `jsdoc/require-param` * Scope to wp-components files only * Scope to tsx files only
What?
Exempts TypeScript files from the
jsdoc/require-paramrule.Why?
TypeScript files generally have their type information in code rather than in JSDoc comments. The
jsdoc/require-paramrule requires all params to be documented in the JSDoc, which would be redundant in these cases.For example,
jsdoc/require-paramwould consider this an error, becausepropsis not documented in the comment:Testing Instructions
.jsfiles.