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(tsdocs): add integration with api-extractor/documenter #2834

Merged
merged 1 commit into from
May 30, 2019

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented May 3, 2019

See #2507

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

@raymondfeng raymondfeng requested a review from bajtos as a code owner May 3, 2019 22:17
@raymondfeng raymondfeng force-pushed the ms-apidocs branch 2 times, most recently from 2f3f2ad to cfeaf26 Compare May 4, 2019 17:18
@raymondfeng raymondfeng force-pushed the ms-apidocs branch 5 times, most recently from 81f2171 to 89e18cc Compare May 6, 2019 03:44
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Thank you for taking a stab at replacing strong-tools with api-extractor!

What's the best way for reviewing the new version of API docs that will be generated with this new tooling?

Are there breaking changes to be aware of? For example, what is the format of anchors and URLs in general? Will we need to update our hand-written docs in docs/site to use the new URL format?

packages/tsdocs/README.md Show resolved Hide resolved
### Generate api docs as markdown files

```sh
npm run generate-apidocs
Copy link
Member

Choose a reason for hiding this comment

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

While this may work inside loopback-next monorepo, I don't think it will work for people installing @loopback/tsdocs into their own projects. I see that the packages is marked as private: true, which means it's not intended for usage outside of loopback-next monorepo. Can you please mention this in package's README too?

If this package is private, how do you envision to generate API docs for experimental packages in loopback-labs? Wouldn't it better if @loopback/tsdocs was a generic tool that can be used in any monorepo? Maybe not, I am just asking.

@bajtos bajtos added feature Internal Tooling Issues related to our tooling and monorepo infrastructore labels May 6, 2019
@raymondfeng
Copy link
Contributor Author

  1. The purpose to have reports and reports-temp is to detect API changes (api reports are tracked by git). See https://api-extractor.com/pages/setup/configure_api_report/.

  2. I'm debating between making @loopback/tsdocs a public module or a just an internal utility. The @loopback/tsdocs can be applied to loopback-labs too as loopback-labs mirrors loopback-next. Let's keep it private for now.

  3. How to preview newly generated apidocs?

  • Check out ms-apidocs branch. Run npm run build:apidocs from inside tsdocs. Use vscode to open index.md in docs/site/apidocs and preview it.

@raymondfeng raymondfeng force-pushed the ms-apidocs branch 3 times, most recently from b61c686 to a91888b Compare May 6, 2019 22:06
@raymondfeng
Copy link
Contributor Author

raymondfeng commented May 6, 2019

Please follow the following instructions to try out new apidocs:

cd loopback-next
git checkout ms-apidocs
npm i
npm run build
npm run tsdocs
cd docs
node ./bin/copy-readmes.js
npm pack

Preview with loopback.io ([email protected]:strongloop/loopback.io.git):

cd loopback.io
npm i
npm i <docs/loopback/loopback-docs-1.16.0.tgz>
npm start

@raymondfeng raymondfeng force-pushed the ms-apidocs branch 2 times, most recently from 397b119 to 500f76f Compare May 7, 2019 04:34
@raymondfeng
Copy link
Contributor Author

Home page:

image

Module:
image

@bajtos
Copy link
Member

bajtos commented May 7, 2019

@raymondfeng how about the format of links - are there any breaking changes?

Are there breaking changes to be aware of? For example, what is the format of anchors and URLs in general? Will we need to update our hand-written docs in docs/site to use the new URL format?

@bajtos
Copy link
Member

bajtos commented May 7, 2019

IIUC, this pull request is entirely reworking the way how API documentation works in LB4 and moves it from http://apidocs.loopback.io to https://loopback.io/doc/en/lb4. I love that ❤️

Having said that, moving API docs is not only about adding markdown files and a new sidebar. Obviously, we need to fix all places in our current documentation that are pointing to http://apidocs.loopback.io and change them to point to the new API doc location. Another place to change is the top header/navbar shown by https://loopbac.io/doc:

Screen Shot 2019-05-07 at 15 58 57

I am not asking to make those changes right now, but we should have a good understanding of what else needs to be done and a list of follow-up tasks to make those change happen.

The purpose to have reports and reports-temp is to detect API changes (api reports are tracked by git). See https://api-extractor.com/pages/setup/configure_api_report/.

+1 to detect API changes.

However, I don't understand why we need to directories for that? Isn't it enough to use git diff? The doc page you provided does not explain change detection, or at least I did not find any explanation.

I'm debating between making @loopback/tsdocs a public module or a just an internal utility. The @loopback/tsdocs can be applied to loopback-labs too as loopback-labs mirrors loopback-next. Let's keep it private for now.

Fair enough.

How to preview newly generated apidocs?
Check out ms-apidocs branch. Run npm run build:apidocs from inside tsdocs. Use vscode to open index.md in docs/site/apidocs and preview it.
Please follow the following instructions to try out new apidocs:
(...)

Thank you for the detailed instruction! I'll try to find some time later this week to play with this. In general and at high level, I am strongly in favor of moving to api-extractor, even if it will require few follow-up pull requests to clean things up.

@strongloop/loopback-maintainers I would like to get few more opinions on the proposed change please. Follow the instructions provided by Raymond above, look around the new API docs, look for unusual or suspicious things.

chore(docs): add apidocs markdown files for loopback.io

Is it really necessary to keep these generated markdown files in git? I am concerned about the noise it will add to our pull request, many of which are already way too large.

So far, we are generating API docs files as part of the release process. I would like to preserve that behavior.

I understand that we need the generated markdown files for Jekyll to render the docs. I believe this can be solved by adding a new build step to ./bin/build-docs-site.sh.

@bajtos
Copy link
Member

bajtos commented May 7, 2019

chore(docs): add reports from api-extractor to track api changes

I think this pull request would be easier to review if this commit was left out. Since those reports are auto-generated, they will be easy to commit in a follow-up pull request.

@raymondfeng
Copy link
Contributor Author

However, I don't understand why we need to directories for that? Isn't it enough to use git diff? The doc page you provided does not explain change detection, or at least I did not find any explanation.

api-extractor uses reports and reports-temp for the comparison. Detected changes are reported as part of the extraction process.

@raymondfeng
Copy link
Contributor Author

how about the format of links - are there any breaking changes?

The links will be changed to doc/en/lb4/apidocs.*.html.

@raymondfeng
Copy link
Contributor Author

I think this pull request would be easier to review if this commit was left out. Since those reports are auto-generated, they will be easy to commit in a follow-up pull request.

I removed commits for generated docs.

@raymondfeng
Copy link
Contributor Author

@bajtos I'm thinking about the following steps to roll out this feature:

  1. Leave current apidocs (strong-docs, apidocs.loopback.io) as is for now.
  2. Land this PR with updated sidebar and generated md files for apidocs so that loopback.io starts to render new style of apidocs.
  3. Review new pages for apidocs, fix issues and improve the look and feel
  4. Switch to new style of apidocs when we are happy
    • Fix links to apidocs.loopback.io
    • ...

@raymondfeng
Copy link
Contributor Author

I understand that we need the generated markdown files for Jekyll to render the docs. I believe this can be solved by adding a new build step to ./bin/build-docs-site.sh.

IIUC, ./bin/build-docs-site.sh. checks out loopback.io repo into sandbox for validation of docs site building. It is not a script used by loopback.io to serve our docs.

Please note that index.md needs to be generated within loopback-next to leverage lerna. Patching md files with Jekyll metadata can be done independently.

@raymondfeng
Copy link
Contributor Author

IMO, seeing diffs in generated apidocs md files for a PR is probably a good side effect. It helps reviewers easily find out if there are api changes.

@raymondfeng
Copy link
Contributor Author

raymondfeng commented May 21, 2019

Parameter documentation is not rendered either.

I tried on https://microsoft.github.io/tsdoc/ and it turned out the @param has to include -:

@param x - The first input number

Without x, the parameter name becomes '' and there is no matching description.

See microsoft/tsdoc#128

@raymondfeng
Copy link
Contributor Author

See #2933

@raymondfeng raymondfeng force-pushed the ms-apidocs branch 2 times, most recently from 5304d47 to 1c1d976 Compare May 22, 2019 18:20
@bajtos
Copy link
Member

bajtos commented May 24, 2019

I just adjusted build:site to generate apidocs before provisioning loopback.io. It should work out of box now

👍

Parameter documentation is not rendered either.
See #2933

Parameter documentation seems ok now.

There is still not documentation for class constructors and their parameters though.

@raymondfeng
Copy link
Contributor Author

There is still not documentation for class constructors and their parameters though.

It's probably an issue with api-extractor/documenter. I'll investigate. But it should not hold up this PR.

@raymondfeng
Copy link
Contributor Author

raymondfeng commented May 25, 2019

Constructor support - microsoft/rushstack#1295

Another PR to make it easier to troubleshoot unknown tags: hhttps://github.com/microsoft/rushstack/pull/1296

The project maintainers don't seem to be very responsive. Based on their disclaimer at https://github.com/microsoft/web-build-tools/tree/master/apps/api-documenter, we might to fork it to make enhancements.

@raymondfeng raymondfeng requested a review from bajtos May 25, 2019 04:10
@raymondfeng raymondfeng force-pushed the ms-apidocs branch 2 times, most recently from cf643d1 to 0af199b Compare May 27, 2019 06:39
@raymondfeng
Copy link
Contributor Author

As microsoft/rushstack#1295 is merged and released, constructor tsdoc is now supported.

@raymondfeng
Copy link
Contributor Author

@bajtos PTAL.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

LGTM as the initial implementation to be improved as we learn more about its limitations.

Please get few more approvals from other team members in @strongloop/loopback-maintainers before landing.

As I commented elsewhere, we should find a way how to programmatically check tsdoc comments for conformity with api-extractor format, so that npm test fails if any of the tsdoc comments are not valid. I think that's best left out of this pull request though.

docs/site/sidebars/lb4_sidebar.yml Show resolved Hide resolved
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT
Object.defineProperty(exports, '__esModule', {value: true});
const pkg2_1 = require('pkg2');
Copy link
Member

Choose a reason for hiding this comment

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

Fair enough.

Please capture this information in developer documentation. For example, you can create packages/tsdocs/fixtures/monorepo/README.md to explain what is in this fixture. Ideally, this documentation should also explain the process for updating the fixture files, should we need to do so in the future.

@bajtos
Copy link
Member

bajtos commented May 30, 2019

The PR description says Fixes #2507. I disagree with that - the spike is done only after we have a list of follow-up tasks. Let's keep #2507 open even after this PR is landed - I am going to edit the PR description.

@bajtos
Copy link
Member

bajtos commented May 30, 2019

@raymondfeng BTW, did you follow all steps listed in Adding a new package?

@raymondfeng raymondfeng force-pushed the ms-apidocs branch 2 times, most recently from 9afabaa to 36f4d15 Compare May 30, 2019 16:40
@raymondfeng
Copy link
Contributor Author

raymondfeng commented May 30, 2019

Please capture this information in developer documentation. For example, you can create packages/tsdocs/fixtures/monorepo/README.md to explain what is in this fixture. Ideally, this documentation should also explain the process for updating the fixture files, should we need to do so in the future.

I added README and made it possible to rebuild the monorepo.

@emonddr emonddr self-requested a review May 30, 2019 19:59
@emonddr
Copy link
Contributor

emonddr commented May 30, 2019

Should files with .../dist/... be included in commit?
e.g.
packages/tsdocs/fixtures/monorepo/packages/pkg1/dist/index.d.ts

@raymondfeng
Copy link
Contributor Author

Should files with .../dist/... be included in commit?
e.g.
packages/tsdocs/fixtures/monorepo/packages/pkg1/dist/index.d.ts

This is part of the fixture for a mock-up monorepo to test out apidocs generation. We commit such files to avoid build steps/costs.

@@ -41,6 +41,7 @@
"version": "npm run update-packages && git add greenkeeper.json \"**/package-lock.json\" && npm run update-template-deps && npm run apidocs",
"outdated": "npm outdated --depth 0 && lerna exec --no-bail \"npm outdated --depth 0\"",
"apidocs": "node bin/run-lerna run build:apidocs",
"tsdocs": "lerna run --scope @loopback/tsdocs build:tsdocs",
Copy link
Contributor

Choose a reason for hiding this comment

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

do we still need command apidocs at the above line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We keep existing strong-docs based apidocs for now. The PR will allow new styles to be seen on loopback.io. Once we are happy with the new approach, we'll remove strong-docs.

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

Cool 🚢 :shipit: LGTM

I run the preview of website and it looks great. One thing to improve(we can do it in following PRs):

Screen Shot 2019-05-30 at 5 33 49 PM

Types like Registry doesn't point to their definitions, which our current doc does, e.g. clicking ApplicationConfig it scrolls to its definition.

@raymondfeng raymondfeng merged commit c8d9572 into master May 30, 2019
@raymondfeng raymondfeng deleted the ms-apidocs branch May 30, 2019 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Internal Tooling Issues related to our tooling and monorepo infrastructore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants