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

feat(compiler): extract doc info for JsDoc #51717

Closed
wants to merge 6 commits into from

Conversation

jelbourn
Copy link
Member

@jelbourn jelbourn commented Sep 9, 2023

Based on top of #51713

This commit adds docs extraction for information provided in JsDoc comments, including descriptions and Jsdoc tags.

jelbourn and others added 5 commits September 9, 2023 11:55
This commit adds a barebones skeleton for extracting information to be used for extracting info that can be used for API reference generation. Subsequent PRs will expand on this with increasingly real extraction. I started with @alxhub's angular#51615 and very slightly polished to get to this minimal commit.
Based on top of angular#51682

This expands on the skeleton previously added to extract docs info for classes, including properties, methods, and method parameters. Type information and Angular-specific info (e.g. inputs) will come in future PRs.
Based on top of angular#51685

This expands on the extraction with information for directives, including inputs and outputs. As part of this change, I've refactored the extraction code related to class and to directives into their own extractor classes to more cleanly separate extraction logic based on type of statement.
Based on top of angular#51697

Adds extraction for accessors (getters/setters), rest params, and resolved type info for everything so far. This also refactors function extraction into a new class and splits tests for common class info and directive info into separate files.
Based on top of #angular#51700

Also updates extraction to ignore un-exported statements.
@jelbourn jelbourn added area: compiler Issues related to `ngc`, Angular's template compiler target: minor This PR is targeted for the next minor release labels Sep 9, 2023
@ngbot ngbot bot added this to the Backlog milestone Sep 9, 2023
@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Sep 9, 2023
* Save some data.
* @param data The data to save.
* @param timing Long description
* with multiple lines.
Copy link
Member

Choose a reason for hiding this comment

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

Can we add another scenario where after the multi-line string another tag is set? Would be good to ensure TS properly handles this. e.g.

        /**
         * Save some data.
         * @param data The data to save.
         * @param timing Long description
         *     with multiple lines.
         * @param bla Long description 2
         *     with multiple times
         */

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, done.

Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM for the JSDoc commit. That's pretty clean- especially if I compare with Dgeni


it('should extract a @see jsdoc tag', () => {
// "@see" has special behavior with links, so we have tests
// specifically for this tag.
Copy link
Member

Choose a reason for hiding this comment

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

what's the special behavior in terms of parsing? is the extra space you're talking about later in the expect?

Copy link
Member Author

Choose a reason for hiding this comment

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

I still need to spend more time figuring out the behavior of @see; I figured it would be more useful to do that later in the process, though, once we're actually doing something to make cross-links work. Trying something like

        /**
         * Future version.
         * @see Component
         */ 

weirdly returns {name: 'see', comment: '*'}, even though that's supposed to be valid JsDoc. For the test case here (@see {@link Component}), comment is actually two separate nodes, presumably one for @link and one for Component, but I wasn't yet able to figure out anything to do with those nodes other than put them back to a string with getTextOfJSDocComment.

@@ -40,10 +40,18 @@ export enum MemberTags {
Output = 'output',
}

export interface JsDocTagEntry {
name: string;
comment: string;
Copy link
Member

Choose a reason for hiding this comment

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

would it be more accurate to name this text/value?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to align with TypeScript's naming here, which is tagName and comment. (the tag part seemed redundant, though)

// In the TS AST, the leading comment for a variable declaration is actually
// on the ancestor `ts.VariableStatement` (since a single variable statement may
// contain multiple variable declarations).
const rawComment = extractRawJsDoc(declaration.parent.parent);
Copy link
Member

Choose a reason for hiding this comment

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

I can't think of a VariableDeclaration outside of a variable list, statement- but would you want to assert this? I think that would make this code more robust and avoid future hard-to-debug scenarios?

Copy link
Member Author

Choose a reason for hiding this comment

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

The type of declaration.parent.parent here is ts.VariableStatement | ts.ForStatement | ts.ForOfStatement | ts.ForInStatement | ts.TryStatement. In the scenario that this code path runs for a for or try statement, I think the current behavior (an empty string if the node doesn't have any JsDoc) is reasonable.

I also expect to need to refine things once I start running it over the real sources.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. Just thought might make things easier to debug if we had an actual runtime assert here

Based on top of angular#51713

This commit adds docs extraction for information provided in JsDoc comments, including descriptions and Jsdoc tags.
jelbourn added a commit to jelbourn/angular that referenced this pull request Sep 12, 2023
Based on top of angular#51717

This commit adds extraction for enums, pipes, and NgModules. It also adds a couple of tests for JsDoc extraction that weren't covered in the previous commit.
jelbourn added a commit to jelbourn/angular that referenced this pull request Sep 15, 2023
Based on top of angular#51717

This commit adds extraction for enums, pipes, and NgModules. It also adds a couple of tests for JsDoc extraction that weren't covered in the previous commit.
jelbourn added a commit to jelbourn/angular that referenced this pull request Sep 15, 2023
Based on top of angular#51717

This commit adds extraction for enums, pipes, and NgModules. It also adds a couple of tests for JsDoc extraction that weren't covered in the previous commit.
pkozlowski-opensource pushed a commit that referenced this pull request Sep 18, 2023
…1733)

Based on top of #51717

This commit adds extraction for enums, pipes, and NgModules. It also adds a couple of tests for JsDoc extraction that weren't covered in the previous commit.

PR Close #51733
@jelbourn
Copy link
Member Author

Changes merged in #51733

@jelbourn jelbourn closed this Sep 18, 2023
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: compiler Issues related to `ngc`, Angular's template compiler detected: feature PR contains a feature commit target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants