Add exported functions option to require-jsdoc rule

[Extersky]

This PR adds support for configuring require-jsdoc rule to require JSDoc comments for exported function bodies only. There are options for configuring the export checks for CommonJS and ES6 exports.
Fixes #192

[Extersky]

There are at least following limitations:

1. Scopes and branches are not supported. This may not be an issue, but may require significant refactoring if desired to be added later
2. The performance would likely improve significantly if a code file wasn't parsed again for each processed node

[brettz9]

I'm not sure what "branches" are in this context (unless it refers to blocks within a scope?), but export isn't allowed anywhere but in the main module scope, so IIUC, I think that is not an issue. (Was forgetting this is for Node modules too, not just ESM.)

From my eyeballing, it looks like this will be quite useful for many projects...

[brettz9]

I've amended my comment, as I was forgetting this is for Node modules , not ESM. But speaking of which, how about amending to have an optional publicFunctionsOnly check for ESM exports? I see it does...

[Extersky]

With branches I meant to refer conditional branching, such as statically analyzable if blocks, which are possible in CommonJS. One such rare case would be a polyfill module where a shim would only be returned if the class/method is not already available. I would consider it ok that for such classes or methods the comments are not enforced, since it can still offer implementation with no JSDoc comment.

[Extersky]

Noticed some more cases where referencing unknown variables would cause errors to be thrown in exportParser.js. The "browserEnv" option also modifies the window variable when the option is not enabled. I'll submit fixes for these today.

[brettz9]

As nice as it is to be able to analyze the whole file, if it wouldn't be too hard to add an option to only check ancestors, I think that would be a nice optimization and possibly the only needed use case for some usage scenarios.

[LukeSkyw]

Very useful PR.

Sorry, I don't know how eslint and its plugin works internally, but I see you are using a custom parser, will that be a problem for Typescript? Would your rule include dangling functions, interfaces, enums, etc

Besides, would your solution worked has it used to on tslint completed-docs rule ? I always got frustrated with this rule because it does not allow precise control on where you do expect documentation and where you don't. For example, how to require documentation on everything on interfaces but to restrict the check to the top level doc for classes (no doc required on methods).

[Extersky]

It is definitely useful to consider how the same rule could be made available for both JavaScript and TypeScript. The typescript-eslint project aims at being able to use many of the same ESLint rules with TypeScript. One obvious option is to implement the same rule in that project separately. Another option is to modify this implementation to make extendable, but I'm not sure if that is reasonable approach.
Does TypeScript have a language service planned or already existing that could provide info about source code exports? In that case the custom parser/resolver implemented here could be replaced.

[brettz9]

Are we ok to merge this as it is now, @Extersky ?

[brettz9]

It'd be nice to get this merged, as I think it should be ready. Anything else @Extersky before sending it on to gajus?

[Extersky]

Sorry, I missed this. From my side merging this is ok in case the optimisation option for only checking through ancestors isn't needed yet.

[Extersky]

The added documentation mentions browser window global export as an option, but this is not yet supported by browserEnv option. Should this be added? This is a simple modification to also check the window variable. I'll start preparing this in case it is included.

[Extersky]

browserEnv option now causes exports to be checked from window global variable. This doesn't yet populate window variable from GeneratorDeclarations, but this isn't currently supported in the rule at all.

[brettz9]

Yeah, sorry, I meant "checks" in the docs, but your docs for it are even better.

This looks great now. I think it is ready to merge. Though it may be helpful if you could offer a summary of any remaining to-dos for the lesser features that are not yet implemented, e.g., checking generator declarations.

@gajus , as a new feature with changes since your approval, do you still want notice? Don't mean to be annoying, just presume it is better to err on the side of caution until I have a better idea of what to pass on, as I took your earlier comment as being about smaller fixes and this is part of a new feature.

[gajus]

@gajus , as a new feature with changes since your approval, do you still want notice? Don't mean to be annoying, just presume it is better to err on the side of caution until I have a better idea of what to pass on, as I took your earlier comment as being about smaller fixes and this is part of a new feature.

I have no issues with you taking the lead. All changes you've pushed that I have reviewed did not ask for a single comment. Just take the lead and ping me if you need any help. I only ask not to do major code structure changes without first raising an issue (such as rewrite in TS, changing ESLint preset, etc).

[brettz9]

Great, sounds good, thanks!

[brettz9]

@Extersky : I've made one other change (discussed off list), re: switching to "esm", "cjs", and "window" for greater clarity on how the export types apply, as "exports" can, as you suggest, be confusable with exports in "cjs". I also thought browserEnv was misleading as well, since we were not checking for the environment; nor do I think we should given that some polyfills may work in a Node environment yet still expect docs on window if they set that option to true.

[gajus]

🎉 This PR is included in version 8.2.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[brettz9]

@Extersky : I just went through the codebase to try to get us to 100% coverage, and though I got through everything else, including some changes to hasReturnValue which would impact require-jsdoc, I was wondering if you might be able to help add tests for those lines in exportParser.js not yet covered in our tests?

Toward this end, I added an npm script, npm run test-cov which gives a printout showing a summary of which lines are still uncovered across all files. (The uncovered lines column (for exportParser.js) is truncated at the beginning, but one can start from the line numbers listed at the end.) If there really is some code which will not be covered now (and which has no public interface that we can add unit tests for), we can disable (with /* istanbul ignore next */), as I did for a handful of lines in jsdocUtils.js since they are private and are not currently possibly to be visited, but which are nevertheless probably wise to keep there as guards, e.g., in case expanded in the future for other types.