docgen: Replace doctrine with comment-parser #28615
Conversation
sarayourfriend
requested review from
adamsilverstein,
ajitbohra,
cameronvoell,
chrisvanpatten,
ellatrix,
guarani,
mkaz,
nerrad,
oandregal and
ntwb
as code owners
|
Size Change: 0 B Total Size: 1.38 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Awesome work. It looks very promising. There are still some issues to resolve but overall it'd be a great improvement as we start using more TypeScript syntax for types.
I left some comments where I could spot something that concerns me and places where you can see TS types properly included.
I would love to hear also from @nosolosw how does he feel about the changes proposed since he authored @wordpress/docgen and he has the most experience here.
docs/designers-developers/developers/data/data-core-keyboard-shortcuts.md
Show resolved
Hide resolved
| @@ -74,7 +74,7 @@ resolvers, | |||
|
|
|||
| _Parameters_ | |||
|
|
|||
| - _paths_ (unknown type): | |||
| - _paths_ ``: | |||
There was a problem hiding this comment.
It still isn't working or it's missing a type or it has wrong type.
There was a problem hiding this comment.
It is missing the type:
gutenberg/packages/data-controls/src/index.js
Line 106 in d9e7541
There was a problem hiding this comment.
Can we remove the @param paths from here? I'm not familiar with this part of the code but the param doesn't make sense as it is.
| - <https://facebook.github.io/react-native/docs/platform-specific-code#platform-module> | ||
|
|
||
| Here is an example of how to use the select method: | ||
| - <https://facebook.github.io/react-native/docs/platform-specific-code#platform-module> Here is an example of how to use the select method: |
There was a problem hiding this comment.
The example intro is bundled as part of the @see tag description while is a separate thing. I presume comment-parser assigns all the text within tags to the previous tag, hence the error.
One thing we can do to fix this is moving the intro text Here is an example of how to use the select method: below the @example tag. That way it works fine.
|
I've only scanned this but it looks really promising. I wanted to share TSDoc which is worth testing and exploring. It may either be usable directly or a good source of inspiration. I think this line is important and represents some common frustrations with JSDoc:
|
Ooooo, I originally started to re-write docgen using Update: @sirreal Unfortunately
Inspecting the result of parsing the comment also reveals that while the parameter is there by name, its type is completely ignored and thrown out. Therefore, it is not possible to use TSDoc, even with significant refactor, for our purposes here. I think the actually very powerful alternative approach for I began exploring this avenue but it would require a significant |
|
I've updated the issue description with other cases in which the docs changed (nullable and optional types, arrays). I wonder if these changes should make it to https://developer.wordpress.org/block-editor/contributors/develop/coding-guidelines/#javascript-documentation-using-jsdoc |
|
This is promising! Thanks for taking up on replacing
Not for this PR, but I have some thoughts on the topic of upgrading / creating a replacement for
|
FWIW, these changes are due to differences in how the types are documented, all the library does is copy the type out of the curly braces (unlike the previous library which attempted to normalize them). If we want to normalize these types, we could add that too but it would take significant work. There's an issue (#22062) to make these decisions that has almost no activity. Personally I think it's a lost cause to try to normalize them during parse time, we might be better off spending that time normalizing the way they're actually recorded in the annotations themselves. |
sarayourfriend
force-pushed
the
try/comment-parser-for-docgen
branch
from
11bc028
to
e49d8b8
Compare
There was a problem hiding this comment.
Thanks for working on this @saramarcondes! This is ready in my view.
There are a few weird cases in which the spacing is removed or the text added to the previous tag. Added some suggestions to fix this, it can be done here or in follow-ups.
We also need to update the changelog for the package release. Sharing my thoughts on this:
This PR replaces doctrine, a deprecated library, with comment-parser. The major change to our public API is that docgen no longer parses and structures the types (because comment-parser doesn't do it), it passes through whatever the author has written:
-
This flexibility works great for types that aren't part of the JSDoc spec. For example, adding some typescript
importsyntax. -
It has the drawback of inconsistency: we are already seeing the effects of this with nullable or optional types, that different people write differently. Those differences are going to increase over time: I wonder if we can do anything to converge syntaxes (lint rule, etc?).
Given this context, it seems to me we should do a major release for docgen. @gziolo @saramarcondes thoughts?
It definitely should be a major version bump. It's a different parser now and it will produce different output in most of the cases 👍🏻 There is one failing unit test that needs to be fixed to unblock merging: |
| @@ -40,7 +40,6 @@ const shouldRenderItem = ( selectedBlocks, allowedBlocks ) => | |||
| * @param {string} [props.role] The ARIA role for the menu item. | |||
| * | |||
| * @example | |||
| * <caption>ES5</caption> | |||
There was a problem hiding this comment.
@nosolosw, wasn't it necessary for the Block Editor Handbook to add tabs to switch between two versions of the example?
There was a problem hiding this comment.
Hmm, I don't see it having any impact on the generated output. So it looks like it was obsolete 😄
There was a problem hiding this comment.
I commented in another thread above I can't find now 😅 That was the original goal but don't remember we ever implemented it.
sarayourfriend
force-pushed
the
try/comment-parser-for-docgen
branch
2 times, most recently
from
5256c2d
to
1bda459
Compare
sarayourfriend
force-pushed
the
try/comment-parser-for-docgen
branch
from
2faae2e
to
dacc9b7
Compare
|
I like all the fixes, I'm happy to approve 😃 @nosolosw, any blockers? |
|
Please, do merge! I had already approved a week ago :D |
Description
Replaces the usage of
doctrinewithcomment-parser.Notable differences:
(string|Object)tostring|Object.Array<Object>toObject[].?typetotype?and that optional types can be either[type]ortype=, depending on how the author declared them in the JSDoc source comment.Notable issues with
comment-parser.as
{ "tag": "return", "name": "The", "type": "string", "optional": false, "description": "description for a return.", }Note that the description of the return is split into two... this appears to be a difference in convention, some folks name their return values. We tend not to do so. Regardless, the library does this for all tags so you'll see something resembling
`${tag.name} ${tag.description}`places where previously we could just use the description. I don't personally see this as an issue, it's just a quirk of the library, but I could see this getting in the way of adopting this approach if folks think it's too fragile or something like that.Notable is that I don't think this is a bug in the library, I think it's just the way it parses based on some conventions. It's a little silly for tags like
@seebut I think it's livable.How has this been tested?
Tests pass and documentation generation still works as expected (with some minor changes given the new parser)
Types of changes
Non-breaking changes
Checklist:
Fixes #26971
Fixes #18045