[api-extractor] Add support for showing whether properties are readonly #3427
Conversation
mrshllstock
requested review from
D4N14L,
apostolisms,
iclanton and
octogonz
as code owners
| const docComment: tsdoc.DocComment | undefined = apiItemMetadata.tsdocComment; | ||
| const declarationMetadata: DeclarationMetadata = this._collector.fetchDeclarationMetadata(astDeclaration); | ||
| return (astDeclaration.modifierFlags & ts.ModifierFlags.Readonly) !== 0 | ||
| || (docComment !== undefined && docComment.modifierTagSet.hasTagName('@readonly')) |
There was a problem hiding this comment.
Suppose someone puts a @readonly tag on an item that cannot be readonly (i.e. they use the tag incorrectly). For example:
class Example {
/** @readonly */
set foo(value: string) { throw new Error('Cannot set foo'); }
}
Suppose set foo has no corresponding getter. It's obviously not readonly, because there's only a setter. But it has the @readonly tag, which means the developer has indicated that it should be displayed as readonly. What should we do in this case?
The logic as we have it will display it as readonly. I think that's probably fine... to defer to the developer's understanding and just let the doc comment tag win. Just thought I'd call out this edge case.
There was a problem hiding this comment.
I'd think the inclusion of the comment that is specifically for documentation would over-ride poor implementation, especially in the context of a documentation generation tool, but in reality the comment has no meaning.
My 2c is that we would still evaluate the comment as isReadonly.
common/changes/@microsoft/api-documenter/api-readonly-mixin_2022-05-19-20-18.json
Outdated
Show resolved
Hide resolved
| return parameters; | ||
| } | ||
|
|
||
| private _determineReadonly(astDeclaration: AstDeclaration): boolean { |
There was a problem hiding this comment.
If this isReadonly flag only applies to properties, property signatures, and variables, does it make sense to inline this logic within each specific "process" method instead of pulling it out into a helper? I don't feel super strongly either way.
There was a problem hiding this comment.
I'm slightly partial towards a helper since the TSDoc comment and getter logic would apply across the board, but I can separate out if greatly desired.
mrshllstock
force-pushed
the
api-readonly-mixin
branch
from
1f8fe77 to
b9d1d35
Compare
common/changes/@microsoft/api-documenter/api-readonly-mixin_2022-05-19-20-18.json
Outdated
Show resolved
Hide resolved
|
@octogonz This PR should be ready for you to take a look. The changes to api-documenter will be in a separate PR (zelliott#2). |
mrshllstock
force-pushed
the
api-readonly-mixin
branch
from
4a939e6 to
a41f3fa
Compare
…mixin # Conflicts: # apps/api-extractor/src/generators/ApiModelGenerator.ts # build-tests/install-test-workspace/workspace/common/pnpm-lock.yaml # common/reviews/api/api-extractor-model.api.md # libraries/api-extractor-model/src/model/ApiProperty.ts
…ause it is already inherited from ApiPropertyItem
|
@mrshllstock Thanks for contributing this feature! I made a few minor changes to your branch:
|
4 participants
Summary
Fixes #3265
Details
Adds a new ApiReadonlyMixin mixin similar to the existing ApiStaticMixin mixin.
This mixin is applied to api-extractor-model as to whether a class member is readonly. This can be via modifier, only having a getter, or TSDoc
How it was tested
Created a new API extractor scenario that should cover the different ways that something can be readonly and verified outputs with rush rebuild.