getJSDocs cleanup #12335
getJSDocs cleanup #12335
Conversation
Fix functions that were unused (getJsDocComments), redundant (append) or badly named (getJSDocCommentsFromText)
Yeah, that name is way too long.
1. Get rid of parent check. 2. Use a couple of nested functions, one of them a recursive worker.
And delete its near-duplicate which was much less correct. The callers that had to switch are slightly more complex and more correct now.
Cache only uses one property now. getJSDocs isn't generic and doesn't take a function parameter any more. The code is overall easier to read.
| return tag; | ||
| } | ||
| } | ||
| export function getJSDocComments(node: Node): string[] { |
There was a problem hiding this comment.
Would getCommentsOfJSDocs be clearer?
| @@ -498,6 +498,7 @@ namespace ts { | |||
| /* @internal */ original?: Node; // The original node if this is an updated node. | |||
| /* @internal */ startsOnNewLine?: boolean; // Whether a synthesized node should start on a new line (used by transforms). | |||
| /* @internal */ jsDocComments?: JSDoc[]; // JSDoc for the node, if it has any. | |||
| /* @internal */ jsDocCache?: (JSDoc | JSDocParameterTag)[]; // JSDoc for the node, plus JSDoc retrieved from its parents | |||
There was a problem hiding this comment.
Maybe add a comment explaining that JSDocParameterTag is treated differently because we want to avoid redoing the mapping from each tag to each parameter declaration.
There was a problem hiding this comment.
I improved the comment to explain that jsDoc[Comments] is the JSDoc syntactically on the node whereas jsDocCache is all applicable docs and @param tags.
| return node && firstOrUndefined(getJSDocTags(node, kind)); | ||
| } | ||
|
|
||
| function getJSDocs(node: Node): (JSDoc | JSDocParameterTag)[] { |
There was a problem hiding this comment.
It is kind of strange that this function is called getJSDocs, but it sometime return a tag. Can we seperate these two things? for example, give parameter declaration nodes a seperate cache just for JSDocParameterTag
There was a problem hiding this comment.
I tried this but didn't want two caches, especially when they are almost the same (both tags and docs have comments). Would a different name help, something like getRelatedJSDoc or getApplicableJSDoc?
|
|
||
| return result; | ||
| cache = concatenate(cache, node.jsDocComments); |
There was a problem hiding this comment.
Now that we have renamed all jsDocComments to just jsDoc, can we do that for Node.jsDocComments as well for consistence? I feel because we call this thing JSDocComment before, we might want to avoid reuse the same name with a different meaning in the new world, per my other comment of using CommentOfJSDoc instead
There was a problem hiding this comment.
That's a good idea. It was too large a change for the main jsdoc PR but it should fit in this one just fine.
|
|
||
| function getJSDocs(node: Node): (JSDoc | JSDocParameterTag)[] { | ||
| let cache: (JSDoc | JSDocParameterTag)[] = node.jsDocCache; | ||
| if (!cache) { |
There was a problem hiding this comment.
When does the cache get cleared?
There was a problem hiding this comment.
Never. Once it's created, it should not be changed. But it's only created if it's requested.
Why would you implement an interface using a *class*?
getJsDocComments, which does something quite different fromgetJSDocComments.getJSDocTypeto get a type node and doesn't traverse jsdoc anymore.getJSDocs, the core of the JSDoc retrieval, now caches retrieved JSDoc on nodes.getCorrespondingJSDocParameterTag.