Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace doctrine with comment-parser. #22061

Closed
wants to merge 23 commits into from

Conversation

sainthkh
Copy link
Contributor

@sainthkh sainthkh commented May 4, 2020

Description

  • Replace doctrine with comment-parser.
  • I focused on replicating doctrine behavior and tried not to break generated docs.

How has this been tested?

Unit tests.

Screenshots

N/A

Types of changes

Bug fix

Things to discuss.

This PR made me think what's the better way to generate types. The discussion points are in #22062.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • [N/A] My code follows the accessibility standards.
  • My code has proper inline documentation.
  • [N/A] I've included developer documentation if appropriate.
  • [N/A] I've updated all React Native files affected by any refactorings/renamings in this PR.

@@ -296,7 +296,7 @@ _Returns_
<a name="getFontSize" href="#getFontSize">#</a> **getFontSize**

Returns the font size object based on an array of named font sizes and the namedFontSize and customFontSize values.
If namedFontSize is undefined or not found in fontSizes an object with just the size value based on customFontSize is returned.
If namedFontSize is undefined or not found in fontSizes an object with just the size value based on customFontSize is returned.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ignored the tab.

@oandregal
Copy link
Member

Nice follow-up @sainthkh, thanks for this. My understanding is that our goal is twofold:

  1. Replace doctrine by something else, which is no longer supported.
  2. Be able to use TypeScript-like comments in the JSDoc.

As I ramp up to test and understand the solution we need, would you share a few cases of the second point so we have a clear measure of success for this? cc @aduth @sirreal as per their TypeScript-foo.

@oandregal
Copy link
Member

oandregal commented May 4, 2020

Follow-up: the only obvious thing I've noticed we need fixing is the use of import(something).Type in @typedef descriptions. Example in findDOMNode: code, current docs. At the moment the API docs say it's an unknown type:

# findDOMNode

Finds the dom node of a React component.

Parameters

* component (unknown type): Component's instance.

What would be a good way to present this kind of type in the docs? I can think of these alternatives:

  1. Relaying whatever the contents of the JSDoc type: * component (import('./react').WPComponent): ...

  2. Just the leaf type: * component (WPComponent): ...

I think I'd go with 2. Paired with adding support for extracting the @typedef definitions to someplace public in our API docs (#15186) it'd make our docs really good.

Are there more options available to us?

@aduth
Copy link
Member

aduth commented May 4, 2020

I think I'd go with 2. Paired with adding support for extracting the @typedef definitions to someplace public in our API docs (#15186) it'd make our docs really good.

I'd agree with this, but heavily preferring if we can move in the direction that the custom types definition can be extracted / included in the documentation (#15186).

@sainthkh
Copy link
Contributor Author

sainthkh commented May 5, 2020

1. About TypeScript types.

I intentionally made A&B type fail because I want this PR to mimic the doctrine behavior as much as possible.

But comment-parser can pass TypeScript types because it doesn't care the validity of a type.

To check it yourself, use the code below:

const parse = require( 'comment-parser' );

const code = `
/**
 *
 * @param {{[P in keyof T]?: T[P]}} test It works. (The internal definition for Partial<T>)
 * @param {++++} val It even passes.
 *
 */
`;

console.log( parse( code )[ 0 ].tags );

And to show those types correctly, we need to decide things in #22062.

We can check the validity of types and show them with @babel/core parse. It'll be implemented in the next PR.

2. About Showing Only the Leaf Types.

Fortunately, jsdoctypeparser supports import() syntax. So, I updated it.

3. About @typedef

I'm planning to implement it after @babel/core type parser+generator is finished. I think adding "Types" section after "API" and generating links to those types is the way to go.

Base automatically changed from master to trunk March 1, 2021 15:43
@sainthkh
Copy link
Contributor Author

Closing because it's fixed #28615

@sainthkh sainthkh closed this May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Docgen /packages/docgen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docgen: remove doctrine
3 participants