Docgen/stop crashing on missing return types #37929
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: 0 B Total Size: 1.13 MB ℹ️ View Unchanged
|
gziolo
added
[Tool] Docgen
|
One of the unit tests related to Docgen fails on CI: https://github.com/WordPress/gutenberg/runs/4796282998?check_suite_focus=true I’m personally fine with the changes proposed, but I don’t know the tool that wel. |
dmsnell
force-pushed
the
docgen/stop-crashing-on-missing-return-types
branch
from
400f5ed to
b7228cd
Compare
|
Thanks @gziolo - one of the tests seemed to be a spurious issue with a mobile dependency; the other was a good catch. I've updated and hopefully everything will pass now. |
sarayourfriend
requested changes
Jan 19, 2022
| @@ -397,7 +397,10 @@ function getFunctionToken( token ) { | |||
|
|
|||
| function getFunctionNameForError( declarationToken ) { | |||
There was a problem hiding this comment.
This particular function has been such a headache to maintain with its many edge cases. Thanks for taking the time to improve it!
| `\n- ${ tag.name.trim() }${ | ||
| tag.description ? ' ' : '' | ||
| }${ tag.description.trim() }`, | ||
| ( tag ) => { |
There was a problem hiding this comment.
Thanks for breaking out the conditional logic in the functions in this file. It improves the readability a lot.
Comment on lines
199
to
205
| it( 'should return null if there is no return type', () => { | ||
| expect( getTypeAnnotation( returnTag, node, 0 ) ).toBeNull(); | ||
| } ); | ||
|
|
||
| it( 'should throw an error if there is no param type', () => { | ||
| expect( () => getTypeAnnotation( paramTag, node, 0 ) ).toThrow( | ||
| "Could not find type for parameter 'foo' in function 'fn'." | ||
| ); | ||
| it( 'should return null if there is no param type', () => { | ||
| expect( getTypeAnnotation( paramTag, node, 0 ) ).toBeNull(); | ||
| } ); |
There was a problem hiding this comment.
Can we add some tests for the case where it will throw? That's still a branch in the code.
There was a problem hiding this comment.
it seems like it might be worth removing the try/catch entirely because it shouldn't crash after fixing the null-dereference bug. in fact I'm not sure how to trigger a crash because I'm having to dive deep into the other getType family functions to try and find an exploit in the code that I could trigger…
There was a problem hiding this comment.
Okay, in that case let's just remove that try/catch altogether as you suggested. If the code fails there then we'll hopefully get a nice bug report with a more descriptive error than the one we're currently spitting out anyway 🙂
There was a problem hiding this comment.
removed the try/catch and had to update other code. I did try a few types that I think should be totally invalid and they don't throw. I think the only thing that did throw was attempting to access the typeAnnotation assuming it was there, then trying to grab the name identifier on a function that doesn't exist for the sake of error reporting. without these I think we're safe apart from unlikely bugs that could always creep up
|
Looks like with the most recent change the doc generation is printing empty type and return value descriptions for functions that don't have any type or return annotations. For example: /**
* Initializes WordPress `postboxes` script and the logic for saving meta boxes.
*/
export function* initializeMetaBoxes() {
// ...
}Results in: ### initializeMetaBoxes
Initializes WordPress `postboxes` script and the logic for saving meta boxes.
_Type_
-
I'm curious why the unit tests didn't fail in this case. It'd be good to add a unit test to cover this case based on the already existing fixture example |
|
Thanks for that catch @sarayourfriend. I tried adding a test case for this and it was pretty hard - I couldn't figure out the right way to do it. I also got lost in the test code. The reason for the extra blank I'm tempted to not add a new test case because in this case it seems like code review was effective at catching this. I'm anxious about spending time working in the test code especially if, as you mentioned, we want to rip out most of this code anyway for a more robust and type-aware version. |
The tests failed because they were verifying that there wasn't any type information |
|
Nice. At least our CI actually verifies when there is a documentation change and luckily changes to |
|
I'm a bit disturbed that editor tests failed on this unrelated PR. |
In certain situations if `docgen` finds a JSDoc with a description for the return value of a function but there's no corresponding type listed for the return value it will error out and say it's required. This requirement introduces a mismatch with how many TypeScript functions will be idiomatically _typed_. In this patch we're reporting those missing types as _unknown_ and letting the `docgen` continue. When missing return types of default exports `docgen` would silently crash without an error message because of a bug when trying to find the name of the surrounding function _when generating the error message_. This is fixed by special-casing default exports alongside named exports.
In the midst of split lines and function calls I found it hard to visualize what the output would look like given the code producing it. In this patch we're expanding some of the inline expressions and abstracting a leading newline in `formatTag` to make the individual formatters look more like what they produce.
dmsnell
force-pushed
the
docgen/stop-crashing-on-missing-return-types
branch
from
123fa11 to
6a147f8
Compare
dmsnell
added a commit
that referenced
this pull request
Jan 21, 2022
…FileSync()) While working through #37929 I noticed that the tests in the `docgen` package read fixture data manually by chaining `JSON.parse()` and `fs.readFileSync()`, often at times separated by a number of lines. In this patch we're replacing those with the more natural `require()` statement which removes noise from the test modules and hopefully will also remove a point of confusion for those coming in to modify it. We shouldn't have to ask, "why is this code doing something that looks normal but is doing it in a notably different manner?" After reviewing the history of the work, initially introduced in #13329, I could find no explanation for the approach and found no discussion about it in the PR.
8 tasks
|
@sarayourfriend the tests are all passing and I think the docs are unchanged. is there anything else you would want from this PR? @jorgefilipecosta added you here for extra eyes, but feel free to ignore if you're busy |
dmsnell
added a commit
that referenced
this pull request
Jan 21, 2022
…FileSync()) (#38148) While working through #37929 I noticed that the tests in the `docgen` package read fixture data manually by chaining `JSON.parse()` and `fs.readFileSync()`, often at times separated by a number of lines. In this patch we're replacing those with the more natural `require()` statement which removes noise from the test modules and hopefully will also remove a point of confusion for those coming in to modify it. We shouldn't have to ask, "why is this code doing something that looks normal but is doing it in a notably different manner?" After reviewing the history of the work, initially introduced in #13329, I could find no explanation for the approach and found no discussion about it in the PR.
8 tasks
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.
Resolves #37766
Description
The
docgenprocess currently fails in a number of circumstances where it's trying to find type information, specifically return-type information, and can't find it. As highlighted in PRs converting modules to TypeScript, this introduces a mismatch in how some idiomatic code will be written and in the case where we can't find type information we should quietly accept that and remove the type information from the generated docs.Eventually it would be nice to get better inference from TypeScript on the types it knows, but some issues around type usefulness remain. For now, this patch makes the process more lenient so that it doesn't hold up other changes and so
that we avoid pushing developers from writing value-lacking types such as
{Object}or{Function}for the sake of getting past the linter.For example, by removing the JSDoc type annotations in #37239 we find the corresponding changes to the
datapackage'sREADME. The JSDoc description still provides the helpful part of documenting the types and has a lower risk chance of getting out of sync with the code than it would be to annotateFunctionor<InnerProps extends HOCProps>(Inner: ComponentType<InnerProps>) => {} extends HOCProps ? ComponentType<...> : ComponentType<...>How has this been tested?
After re-building the package the reference docs were re-generated and no differences in output were found against
trunk(given that we don't introduce any of the cases that would have broken thedocgenin this PR we expect no changes).Types of changes
This changes the build pipeline, namely it relaxes the requirement that a JSDoc entry for a value that has a description also has an explicit type listed.
Checklist:
*.native.jsfiles for terms that need renaming or removal).