Docgen: Fix issue where function token can't be found #49812
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +259 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
noahtallen
added
[Type] Build Tooling
noahtallen
changed the title
|
Flaky tests detected in ea37f39. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4694539124
|
There was a problem hiding this comment.
@noahtallen Have you tried to add a test case for this? It looks like a variation of should get the member type for the param, if I'm understanding the problem correctly.
|
Will do, thanks for the tip! |
|
@sarayourfriend I looked into the test, and that helped me find the actual root cause, which was related to how JSDoc parameters are indexed for qualified/unqualified types! I was also able to add a unit test for this behavior. 👍 |
|
@noahtallen I think it's a good fix. That being said, here's an absolute tangent: Have we ever considered switching to https://typedoc.org/? @gziolo recommended it once to me and I moved WordPress Playground from a custom documenter to TypeDoc and the results are splendid. It works like a charm even for the weirdest TS corner-cases and when it fails, it fails gracefully and still produces some output instead of an error. Meanwhile, docgen often confuses us with these errors demanding a hotfix. Typedoc can output markdown, support arbitrary markdown pages, and is extensible with plugins. Is there any real upside to maintaining our own documentation tool? Github search doesn't yield too many results. @youknowriad or @gziolo – can you remember any discussions? |
|
That sounds awesome! I unfortunately haven't been involved in any docgen discussions before; this is just a one-off fix and my first time touching the code. 🙃 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
I encountered an issue in both #49649 and #49651 where the documentation generator couldn't find prop names. For example, I could have code similar to this:
Note: this error does not get triggered when props and props.bar are given types in JSDoc. but this is redundant with types specified in TS.
And I get an error like "Could not find corresponding parameter token for documented parameter 'props.bar'"
An example can be found in this GH actions run: https://github.com/WordPress/gutenberg/actions/runs/4694571924/jobs/8322855362?pr=49651#step:7:27
This is confusing, because it's not an issue everywhere. Just in certain typescript files I've been migrating. I don't understand why it happens, so this PR is an attempt at a fix.The root cause is the logic which processes JSDoc tags, which doesn't pass the correct index for object parameters.
For context, if the function is
function foo( a, b ), then you would have two JSDoc params for both a and b, which are in order. The logic correctly increments an index to connect the JSDoc param "a" with the parsed types for the first argument offoo.However, when you have object parameters that are destructured (common in React components), you have the above example with
propsandprops.bar. The JSDoc param "props.bar" is the second param, but the type information for it is in the first function argument, since it's a property of the object argument.While the logic correctly only increments this index for "unqualified props" (e.g. props without a period in the name), there is a subtle bug where the incorrect index is still passed for qualified props.
When it processes "props", the index is incremented to 1, while 0 is passed, which is correct. Then it processes "props.bar". It doesn't increment the index, but the index is 1 from the previous step. 0 should actually be passed to access the first argument of the function.
If it were to process a 3rd tag, "bar" (unqualified), the index would be incremented to 2, while 1 would be passed, which would still be correct.
So the bug is just with the index for qualified parameters.
Beyond that, this bug never shows up when the type is included in the JSDoc comment, because the index doesn't do anything in that scenario.
Why?
see above
How?
When processing a qualified parameter (e.g. 'props.bar'), pass an index that's less by one.
Testing Instructions
difficult to test locally, but you can basically verify this change by copying these changes to a PR like #49651, and attempt to generate documentation for a change to rich-text/src/is-collapsed.ts