[api-documenter/-extractor/-model] Support 'abstract' Modifier by fwienber · Pull Request #3662 · microsoft/rushstack

Frank Wienberg (fwienber) · 2022-09-29T09:36:06Z

Summary

Proposal implementation of issue #3661.

Fully supporting the abstract modifier in api-documenter involves extending the model (api-extractor-model) and letting api-extractor extract the information into the model.

Then, api-documenter can check whether API items have the new ApiAbstractMixin and if the new isAbstract property is true, add the modifier to the output. On the fly, the order of modifiers has been slightly adjusted to match the required order in TypeScript syntax.

For abstract classes, instead of adding a "Modifier" column for just this one modifier, a dedicated table has been added, because as an API designer, I think of abstract classes as a third category in between interfaces and (non-abstract) classes. Like interfaces, they cannot be instantiated, but like classes, you can subclass them.

	use as type	subclass	instantiate
interface	✅	❌	❌
abstract class	✅	✅	❌
class	✅	✅	✅

Details

There are separate commits for the changes in api-extractor(-model) and api-documenter.
Adding a property to the model affects many generated *.api.json model files.

The last commit implements the second alternative mentioned in #3661: Instead of adding a "Modifiers" column to the "Classes" overview table, a separate table for "Abstract Classes" is added to the Markdown output.

Note that other output targets beyond Markdown are not yet handled explicitly. Please advise if there is anything left to do.

How it was tested

Tests for the changes have been complemented in an existing test scenario (api-extractor) or added as a new one (api-documenter). A new API schema version has been added. (This also updates several *.api.json model files.)

A change log entry has been added.

Analogous to other ApiItem mixins, the new ApiAbstractMixin adds 'isAbstract' as a property aspect. It is mixed into ApiClass and ApiMethod, the only models that may have an 'abstract' modifier. ApiModelGenerator detects the modifier and adds it to the models. This change updates all generated *.api.json files to include the new "isAbstract" property.

Generate "abstract" if an API item has the ApiAbstractMixin and 'isAbstract' is true. So far, this only works for methods, since the classes overview table does not have a "Modifiers" column.

Split classes into abstract and non-abstract classes and add a separate overview table for "Abstract Classes". The alternative would be to add a "Modifiers" column to the existing "Classes" overview table.

Zack Elliott (zelliott)

Looks great! Two final comments;

You'll want to bump the .api.json version in DeserializerContext.ts. See previous edits to that file for examples.
It'd be great to have at least one test in build-tests/ where isAbstract is true. If we already have one - great - if not, let's add one.

Frank Wienberg (fwienber) · 2022-10-11T11:45:11Z

Just FYI, I did not forget about this PR, but I'm busy with another project for a couple of days.

@zelliott

…also for ApiProperty When adding 'abstract' for classes and methods, @zelliott mentioned that properties can have this modifier, too. Thus, ApiProperty must be extended just like ApiMethod and ApiClass, and ApiModelGenerator must extract the 'abstract' modifier into the ApiProperty model. Expected test results change accordingly.

Frank Wienberg (fwienber) · 2022-10-18T11:44:55Z

It'd be great to have at least one test in build-tests/ where isAbstract is true. If we already have one - great - if not, let's add one.

There already was an abstract class with an abstract method, now resulting in an "isAbstract": true entry in the expected API JSON.
Do you think we need to complement test data for an abstract property?

…ract-modifier # Conflicts: # apps/api-extractor/src/generators/ApiModelGenerator.ts

The support to track and render 'abstract' modifiers on classes, methods, and properties affects api-extractor-model, api-extractor, and api-documenter.

Unlike other languages like Java, in TypeScript, the order of modifiers is defined strictly. Currently, it looks like api-documenter uses alphabetical order, which is not the correct TypeScript modifier order. It seems the correct order is (`public` | `protected` | `private`)? (`static`? | `abstract`?) `override`? `readonly`? No member can be `static` and `abstract` at the same time, so the order of these two does not matter. api-documenter currently only uses `protected`, `static`, `abstract` (new in this PR) and `readonly`, so `public` (default!), `private` (no API!) and `override` (maybe interesting for navigation, but not as a modifier) do not apply. Added a code comment about the order and moved `readonly` to the last position so that the overall modifier output order now matches TypeScript syntax.

Frank Wienberg (fwienber) · 2022-10-18T13:10:07Z

I guess I now addressed all review issues and thus resolved the conversations above. Zack Elliott (@zelliott), please re-review and of course feel free to re-open any resolved conversation to comment!

Frank Wienberg (fwienber) · 2022-10-18T15:19:44Z

If you don't like the merge commit that resolves the conflict with "main", I can also rebase my branch, but didn't want to do so while there are comments on the commits that would be lost when force-pushing the rebased branch.

The schema version was updated from 1010 to 1011 in 2088fd3, so this changed should be squashed with 2088fd3.

Frank Wienberg (fwienber) · 2022-10-18T16:18:31Z

I had a bit more time and added more tests:

added an abstract property to api-extractor-scenarios
added an abstract class with an abstract method & property to api-documentor-scenarios

Diff in regenerated report files looks good.

Frank Wienberg (fwienber) · 2022-10-24T08:00:31Z

ping

Zack Elliott (zelliott) · 2022-10-26T01:34:36Z

Hoping to take a look this weekend... maybe sooner. Sorry for the delay!

Zack Elliott (zelliott) · 2022-11-10T16:29:33Z

There already was an abstract class with an abstract method, now resulting in an "isAbstract": true entry in the expected API JSON.
Do you think we need to complement test data for an abstract property?

No, I think that's good enough then. Thanks for checking!

Zack Elliott (zelliott)

Sorry for the very long delay, have been busy with other work (as we all are!). LGTM!

Zack Elliott (zelliott) · 2022-11-30T01:51:01Z

Zack Elliott (@zelliott) Sorry, this time I missed the notification, then was out of office last week. I moved the test (last unresolved conversation) as you recommended. Next step is to find a code owner for review?

Yep! LGTM

Frank Wienberg (fwienber) · 2022-11-30T08:51:51Z

So how is the process here? Are the four mentioned code owners supposed to notice that this PR is ready for review, or do I have to ping (any of) them?

Zack Elliott (zelliott) · 2022-11-30T15:22:02Z

I don't think there's any well defined process unfortunately, Pete Gonzalez (@octogonz) to correct me otherwise. I typically ping Pete Gonzalez (@octogonz) over Zulip chat or on the PR itself.

Frank Wienberg (fwienber) · 2022-12-06T15:58:26Z

Okay, magic spell "ping Pete Gonzalez (@octogonz)" 🧞

…ort-abstract-modifier

Pete Gonzalez (octogonz) · 2023-01-25T06:53:35Z

Sorry I overlooked this PR! 😅 (In the future, you can ping me personally on Zulip if needed.)

Frank Wienberg (fwienber) added 3 commits September 29, 2022 11:03

[api-documenter] Add 'abstract' to "Modifiers" column

b146047

Generate "abstract" if an API item has the ApiAbstractMixin and 'isAbstract' is true. So far, this only works for methods, since the classes overview table does not have a "Modifiers" column.

[api-documenter] Separate "Abstract Classes" overview table

66932f6

Split classes into abstract and non-abstract classes and add a separate overview table for "Abstract Classes". The alternative would be to add a "Modifiers" column to the existing "Classes" overview table.

Frank Wienberg (fwienber) mentioned this pull request Sep 29, 2022

[api-documenter] 'abstract' missing in "Methods" overview table's "Modifiers" column #3661

Closed

Zack Elliott (zelliott) reviewed Oct 6, 2022

View reviewed changes

Comment thread libraries/api-extractor-model/src/mixins/ApiAbstractMixin.ts

Zack Elliott (zelliott) reviewed Oct 6, 2022

View reviewed changes

Comment thread apps/api-documenter/src/documenters/MarkdownDocumenter.ts

Zack Elliott (zelliott) reviewed Oct 6, 2022

View reviewed changes

Comment thread apps/api-documenter/src/documenters/MarkdownDocumenter.ts

Zack Elliott (zelliott) reviewed Oct 6, 2022

View reviewed changes

Frank Wienberg (fwienber) added 3 commits October 18, 2022 14:17

Merge remote-tracking branch 'origin/main' into fwienber/support-abst…

2c272dc

…ract-modifier # Conflicts: # apps/api-extractor/src/generators/ApiModelGenerator.ts

Add new ApiJsonSchemaVersion (V_1011) for 'isAbstract' field

2088fd3

Add change log entries for new 'isAbstract' field

dc41cb5

The support to track and render 'abstract' modifiers on classes, methods, and properties affects api-extractor-model, api-extractor, and api-documenter.

Frank Wienberg (fwienber) marked this pull request as ready for review October 18, 2022 12:34

Frank Wienberg (fwienber) requested review from Daniel (D4N14L), Apostolis Haitalis (apostolisms), Ian Clanton-Thuon (iclanton) and Pete Gonzalez (octogonz) as code owners October 18, 2022 12:34

Regenerate expected test data after updating schemaVersion

c1e886a

The schema version was updated from 1010 to 1011 in 2088fd3, so this changed should be squashed with 2088fd3.

Frank Wienberg (fwienber) requested a review from Zack Elliott (zelliott) October 19, 2022 06:05

Zack Elliott (zelliott) reviewed Nov 10, 2022

View reviewed changes

Comment thread build-tests/api-documenter-scenarios/src/inheritedMembers/index.ts Outdated

Zack Elliott (zelliott) approved these changes Nov 10, 2022

View reviewed changes

Pete Gonzalez (octogonz) added 2 commits January 24, 2023 22:39

Merge remote-tracking branch 'remotes/origin/main' into fwienber/supp…

73cd30e

…ort-abstract-modifier

Update change logs

06c94ac

rush build

c61ad43

Pete Gonzalez (octogonz) approved these changes Jan 25, 2023

View reviewed changes

Pete Gonzalez (octogonz) merged commit e75bca0 into microsoft:main Jan 25, 2023

Conversation

Frank Wienberg (fwienber) commented Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details

How it was tested

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Zack Elliott (zelliott) left a comment

Choose a reason for hiding this comment

Uh oh!

Frank Wienberg (fwienber) commented Oct 11, 2022

Uh oh!

Frank Wienberg (fwienber) commented Oct 18, 2022

Uh oh!

Frank Wienberg (fwienber) commented Oct 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Frank Wienberg (fwienber) commented Oct 18, 2022

Uh oh!

Frank Wienberg (fwienber) commented Oct 18, 2022

Uh oh!

Frank Wienberg (fwienber) commented Oct 24, 2022

Uh oh!

Zack Elliott (zelliott) commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zack Elliott (zelliott) commented Nov 10, 2022

Uh oh!

Uh oh!

Zack Elliott (zelliott) left a comment

Choose a reason for hiding this comment

Uh oh!

Zack Elliott (zelliott) commented Nov 30, 2022

Uh oh!

Frank Wienberg (fwienber) commented Nov 30, 2022

Uh oh!

Zack Elliott (zelliott) commented Nov 30, 2022

Uh oh!

Frank Wienberg (fwienber) commented Dec 6, 2022

Uh oh!

Pete Gonzalez (octogonz) commented Jan 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Frank Wienberg (fwienber) commented Sep 29, 2022 •

edited

Loading

Frank Wienberg (fwienber) commented Oct 18, 2022 •

edited

Loading

Zack Elliott (zelliott) commented Oct 26, 2022 •

edited

Loading