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鈥檒l 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

Projects
None yet
4 participants
@raymondfeng
Copy link
Member

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

@raymondfeng raymondfeng force-pushed the ms-apidocs branch 2 times, most recently from 2f3f2ad to cfeaf26 May 4, 2019

@raymondfeng raymondfeng referenced this pull request May 5, 2019

Merged

chore: fix invalid tsdoc tags in comments #2836

3 of 7 tasks complete

@raymondfeng raymondfeng force-pushed the ms-apidocs branch 5 times, most recently from 81f2171 to 89e18cc May 5, 2019

@bajtos
Copy link
Member

left a comment

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?

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

```sh
npm run generate-apidocs

This comment has been minimized.

Copy link
@bajtos

bajtos May 6, 2019

Member

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.

@raymondfeng

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

  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 May 6, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member Author

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 (git@github.com: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 May 6, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

Home page:

image

Module:
image

@bajtos

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

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

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

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

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

@raymondfeng raymondfeng force-pushed the ms-apidocs branch from 500f76f to 35d1490 May 7, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

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

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

@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

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

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

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

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

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

See #2933

@raymondfeng raymondfeng force-pushed the ms-apidocs branch 2 times, most recently from 5304d47 to 1c1d976 May 22, 2019

@bajtos

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

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

This comment has been minimized.

Copy link
Member Author

commented May 25, 2019

Constructor support - microsoft/web-build-tools#1295

Another PR to make it easier to troubleshoot unknown tags: hhttps://github.com/microsoft/web-build-tools/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

@raymondfeng raymondfeng force-pushed the ms-apidocs branch 2 times, most recently from cf643d1 to 0af199b May 27, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member Author

commented May 27, 2019

As microsoft/web-build-tools#1295 is merged and released, constructor tsdoc is now supported.

@raymondfeng

This comment has been minimized.

Copy link
Member Author

commented May 27, 2019

@bajtos PTAL.

@raymondfeng raymondfeng force-pushed the ms-apidocs branch from 0af199b to 8205629 May 28, 2019

@bajtos

bajtos approved these changes May 30, 2019

Copy link
Member

left a comment

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.

Show resolved Hide resolved docs/site/sidebars/lb4_sidebar.yml
// 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');

This comment has been minimized.

Copy link
@bajtos

bajtos May 30, 2019

Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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 May 30, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member Author

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.

@raymondfeng raymondfeng force-pushed the ms-apidocs branch from 36f4d15 to 431f048 May 30, 2019

@emonddr emonddr self-requested a review May 30, 2019

@emonddr

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

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",

This comment has been minimized.

Copy link
@jannyHou

jannyHou May 30, 2019

Contributor

do we still need command apidocs at the above line?

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng May 30, 2019

Author Member

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.

@jannyHou
Copy link
Contributor

left a comment

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

4 checks passed

clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 91.917%
Details

@raymondfeng raymondfeng deleted the ms-apidocs branch May 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.