-
Notifications
You must be signed in to change notification settings - Fork 604
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
[api-documenter] Support linked types for markdown output #1849
Conversation
Awesome! I'll take a look at this ASAP. |
@@ -17,11 +17,11 @@ export interface IDocInterface6 | |||
| Property | Type | Description | | |||
| --- | --- | --- | | |||
| [arrayProperty](./api-documenter-test.idocinterface6.arrayproperty.md) | <code>IDocInterface1[]</code> | | | |||
| [intersectionProperty](./api-documenter-test.idocinterface6.intersectionproperty.md) | <code>IDocInterface1 & IDocInterface2</code> | | | |||
| [intersectionProperty](./api-documenter-test.idocinterface6.intersectionproperty.md) | [IDocInterface2](./api-documenter-test.idocinterface2.md) | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Feiyang1 this test case is not being handled properly
| [regularProperty](./api-documenter-test.idocinterface6.regularproperty.md) | <code>number</code> | Property of type number that does something | | ||
| [tupleProperty](./api-documenter-test.idocinterface6.tupleproperty.md) | <code>[IDocInterface1, IDocInterface2]</code> | | | ||
| [typeReferenceProperty](./api-documenter-test.idocinterface6.typereferenceproperty.md) | <code>Generic<IDocInterface1></code> | | | ||
| [unionProperty](./api-documenter-test.idocinterface6.unionproperty.md) | <code>IDocInterface1 | IDocInterface2</code> | | | ||
| [unionProperty](./api-documenter-test.idocinterface6.unionproperty.md) | [IDocInterface2](./api-documenter-test.idocinterface2.md) | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Feiyang1 also this test case is not being handled properly
private _tryCreateLinkTagForTypeExcerpt(excerpt: Excerpt): DocLinkTag | undefined { | ||
const configuration: TSDocConfiguration = this._tsdocConfiguration; | ||
// Use the last token, so it works for namespace types, e.g. types.MyType | ||
const typeExcerptToken: ExcerptToken = excerpt.tokens[excerpt.tokenRange.endIndex - 1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Feiyang1 you need to go loop through the individual tokens and only hyperlink the tokens with ExcerptTokenKind.Reference
.
The DocFX/YAML documenter's _renderType() is not a good example to work from, because the YAML file has a weird specialized representation for "complex" types. Whereas for the Markdown documenter, we're just making plain text with some hyperlinks inside.
This means that _tryCreateLinkTagForTypeExcerpt()
probably needs to return a DocParagraph
instead of a simple DocLinkTag
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a sketch of how to fix that: a12b04b
It might need some tuning to handle different ways that whitespace can end up in the DocCodeSpan
parts. I noticed that the original implementation was trim()
ing them for some reason. (?)
Hope that helps!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that makes sense! I didn't think about composite types. Your sketch is very helpful - I copy/pasted it into this PR.
I believe the original implementation trim()
s text for return type because it allows to write the one liner that handles both empty string and only whitespace cases. It probably doesn't matter format wise, so I think we should be good without trim()
ing.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Feiyang1 Looking at some examples, I'm thinking maybe we should switch to plain text instead of code blocks for these type excerpts.
Option 1: DocCodeSpan (original approach)
Consider this example generated by your current PR (using DocCodeSpan
):
f() function
An example function.
Signature:
export declare function f(a: Apple | Banana, b: Map<Apple, number>, y: string | number, z: Symbol): Apple & Banana;Parameters
Parameter Type Description a Apple |
Bananatwo hyperlinkable types b Map
<
Apple, number>
one hyperlinkable type in a generic type y string | number
z Symbol
Returns:
two hyperlinkable types as the return value
Option 2: DocPlainText
And here's how it would look with DocPlainText
:
f() function
An example function.
Signature:
export declare function f(a: Apple | Banana, b: Map<Apple, number>, y: string | number, z: Symbol): Apple & Banana;Parameters
Parameter Type Description a Apple | Banana two hyperlinkable types b Map<Apple, number> one hyperlinkable type in a generic type y string | number z Symbol Returns:
two hyperlinkable types as the return value
I think the second way might actually look better. What's your opinion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Option 3: links inside <code>
A third possibility would be to use markdown links inside <code>
tags, like this:
<b>Returns:</b>
<code>[Apple](./api-documenter-test.apple.md) & [Banana](./api-documenter-test.banana.md)</code>
GitHub renders it like this:
f() function
An example function.
Signature:
export declare function f(a: Apple | Banana, b: Map<Apple, number>, y: string | number, z: Symbol): Apple & Banana;Parameters
Parameter Type Description a Apple | Banana
two hyperlinkable types b Map<Apple
, number>
one hyperlinkable type in a generic type y string | number
z Symbol
Returns:
two hyperlinkable types as the return value
It's definitely not standard Markdown, but a lot of other popular markdown engines seem to support it:
- Marked renders it correctly
- markdown-it renders it correctly, but only if the "HTML tags" option is enabled
- Kramdown (used by Jekyll) doesn't seem to support this at all
Right now I'm leaning towards Option 2 DocPlainText.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Feiyang1 here is an implementation of Option 2: Feiyang1#1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the output of Option 3, but it's unfortunately not standard markdown. So yeah, Option 2 seems to be the most reasonable choice.
I went ahead and resolved the merge conflict in your PR. Feel free to merge.
…rosoft/tsdoc` library is used
I have regenerated the Rush Stack API Docs using this PR. Definitely looks better! 😁 |
This fix was published with API Documenter 7.8.0 |
@Feiyang1 FYI we mentioned you in the announcement: https://twitter.com/rushstack/status/1258082338929434632 |
Woohoo! Thank you for the thorough feedback, adding tests and writing code directly! api-extractor/api-documenter is a great combo for creating reference docs and other things for Typescript libraries (maybe the best right now). Keep up the good work! |
Fixes #1611
This adds support for linked types for type cells and return types for markdown output.
I followed @octogonz's recommendation by creating a map in
ApiModel
to trackcanonicalReference
-->ApiItem
, then conditionally creatingDocLinkTag
for types if the correspondingApiItem
is found in the map.Let me know if/where tests need to be added. Will be happy to do that.