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

docs(headless): api-extractor doc generation PoC #476

Merged
merged 24 commits into from
Feb 10, 2021
Merged

docs(headless): api-extractor doc generation PoC #476

merged 24 commits into from
Feb 10, 2021

Conversation

samisayegh
Copy link
Contributor

This PR shows a proof-of-concept for generating documentation for the Pager controller. I checked in the final JSON to help visualize the output of the parsing step.

General

I found api-extractor easier to work with compared to typedoc. It's a tool that is focused on our exact use-case (i.e. extracting documentation to present in a custom template). The JSON was easier to navigate. There is also a parser for the JSON that made post-processing much easier. Not needing to write a parser allows us to benefit from the project over time as the team at Microsoft develops it.

One draw-back was api-extractor cannot extract docs from inside a controller, or on types. This means we have to use interfaces and write out the properties. We cannot use typescript inference (e.g. type Pager = ReturnType). One positive is it makes post-processing simpler. Typedoc was able to extract everything, it generated a JSON was complex to parse. One negative is there will be repetition of doc strings for certain controllers (e.g. facets, search-box/standalone search-box).

Notes

  • There is a rollup-plugin-dts step to resolve import() types that api-extractor cannot parse. There is an open issue ([api-extractor] import() types are not handled correctly in .d.ts rollups microsoft/rushstack#1050) to add support, but until this happens, we need this additional step.
  • The "DOM" lib in tsconfig is needed to resolve two browser types in the platform-client: AbortController and Response.
  • The docs on the Pager controller are now on the interface instead of inside the controller function so that api-extractor includes them in the JSON.

type: string;
isOptional: boolean;
desc: string;
// notes?: []
Copy link
Contributor Author

@samisayegh samisayegh Feb 2, 2021

Choose a reason for hiding this comment

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

This property would be to include things like default, min, max, beta etc. I did not support it for the PoC.

Copy link
Member

Choose a reason for hiding this comment

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

You mean when we annotate @default 5 in the JSDoc, it would be "leveraged" by notes ?

How would that be leveraged exactly ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this script should ultimately include the tags and their values as part of the parsed object and the outputed JSON. From there, the ruby scripts and templates inside the jekyll doc repo would be free to represent the information as they wish, whether in a table or some other structure.

@@ -16,15 +16,18 @@
"scripts": {
"start": "npx concurrently \"npm run typedefinitions -- -w\" \"rollup -c -w\"",
"build": "npm run clean && npm run build:prod",
"build:prod": "rollup -c --environment BUILD:production && npm run typedefinitions",
"build:prod": "npm run typedefinitions && rollup -c --environment BUILD:production",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typedefinitions are generate before the rollup step to resolve import types.

Copy link
Contributor

@kaitlynkatz kaitlynkatz left a comment

Choose a reason for hiding this comment

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

This looks solid to me. We'll need to do some processing on the desc field, but it seems to contain everything we need. I also think testing it on the Facet controllers before we get too deep into this strategy would be worthwhile, since that's a main sticking point at the moment.

@@ -57,20 +57,57 @@ const initialStateSchema = new Schema({
page: new NumberValue({min: 1}),
});

export type Pager = ReturnType<typeof buildPager>;
export interface Pager {
Copy link
Contributor

@ThibodeauJF ThibodeauJF Feb 3, 2021

Choose a reason for hiding this comment

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

The mass interface edit is certainly gonna be painful but if it's necessary 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

How does it affect intellisense in vscode ?

Is there any major difference, or is it essentially the same ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intellisense is identical.

Ideally, I would have liked to keep the freedom to use any of typescript's features, but the ts compiler is ahead of the documentation tools atm, which are either unable to resolve the documentation (e.g. api-extractor) or resolve the documentation but output a JSON with all the intermediate steps (typedoc).

Copy link
Member

@olamothe olamothe left a comment

Choose a reason for hiding this comment

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

I guess it's more a question for @kaitlynkatz , but when this is merged, this would most probably break some system in the documentation website, right ?

Any coordination that needs to happen between the two ?

type: string;
isOptional: boolean;
desc: string;
// notes?: []
Copy link
Member

Choose a reason for hiding this comment

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

You mean when we annotate @default 5 in the JSDoc, it would be "leveraged" by notes ?

How would that be leveraged exactly ?

@@ -57,20 +57,57 @@ const initialStateSchema = new Schema({
page: new NumberValue({min: 1}),
});

export type Pager = ReturnType<typeof buildPager>;
export interface Pager {
Copy link
Member

Choose a reason for hiding this comment

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

How does it affect intellisense in vscode ?

Is there any major difference, or is it essentially the same ?

@samisayegh
Copy link
Contributor Author

We'll need to do some processing on the desc field, but it seems to contain everything we need.

Indeed, this is an area to dig into. The JSON parser actually parses the tsdoc into a tree of nodes, making it possible to retrieve sections such as summary, remarks, returns, tags, etc. However, I did not find an easy way to reconstruct the nodes to a string. We may need to write something to help with this. Ideally though, we should process everything at this stage so the ruby scripts do not have to worry about this.

I also think testing it on the Facet controllers before we get too deep into this strategy would be worthwhile, since that's a main sticking point at the moment.

Yep, I will tackle the regular Facet next,

@github-actions
Copy link

github-actions bot commented Feb 3, 2021

Pull Request Report

PR Title

✅ Title follows the conventional commit spec.

Bundle Size

File Compression Old (kb) New (kb) Change (%)
dist/browser/headless.js bundled 435.8 432.8 -0.7
minified 178.9 178.9 0
gzipped 51.5 51.5 0
../atomic/src/external-builds/headless.esm.js bundled 424.9 421.9 -0.7
minified 227.3 227.3 0
gzipped 56.7 56.7 0
dist/browser/headless.esm.js bundled 424.9 421.9 -0.7
minified 227.3 227.3 0
gzipped 56.7 56.7 0

@samisayegh samisayegh merged commit e518db3 into master Feb 10, 2021
@samisayegh samisayegh deleted the KIT-402 branch February 10, 2021 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants