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: sprucing up docs, esp. reference doc generation #2070

Closed
wants to merge 18 commits into from
Closed

Conversation

filmaj
Copy link
Contributor

@filmaj filmaj commented Oct 3, 2024

  • dynamically load generated sidebar items - requires a bit of gymnastics, because TypeDoc, which imports+parses TypeScript code to generate docs from source code comments, runs in parallel during docusaurus sidebar generation. As a result, sometimes there can be race-condition-like errors where the TypeDoc generation is not complete before the sidebar generation kicks off. Code was added to, basically, wait for the relevant sub-package TypeDoc routines to complete before finalizing the sidebar items.
  • add biome / linting to docs folder
  • lint during docs CI
  • tweak generated reference documentation styles (tables instead of lists where possible)

Doing this as part of a an initial pass to learn the docusaurus ropes as I inch towards applying the same thing to bolt-js docs.

I left some in-line comments with screenshots of relevant changes. The big visual change is using tables for API references instead of bulleted lists: I think this is more visually pleasing and also scalable as we add more content to the docs.

Filip Maj added 2 commits October 3, 2024 15:40
…o docs folder, lint during docs CI, tweak generated reference documentation styles (tables instead of lists where possible).
@filmaj filmaj added docs M-T: Documentation work only semver:patch tests M-T: Testing work only area:typescript issues that specifically impact using the package from typescript projects labels Oct 3, 2024
@filmaj filmaj requested review from lukegalbraithrussell and a team October 3, 2024 19:45
@filmaj filmaj self-assigned this Oct 3, 2024
docs/docusaurus.config.js Outdated Show resolved Hide resolved
docs/docusaurus.config.js Outdated Show resolved Hide resolved
*/
function packageReferenceOptions(pkg) {
return {
classPropertiesFormat: 'table',
Copy link
Contributor Author

@filmaj filmaj Oct 3, 2024

Choose a reason for hiding this comment

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

The various *Format properties and setting them to table, I think, is nicer than the default of a bulleted list - makes better use of horizontal space, and as we add more kinds of information to our source code comments / JSdocs, these should show up as separate columns. E.g. if we add an @examples JSDoc tag to some method docs, that will show up as a new column in the generated docs.

An example of a table this setting outputs:

Screenshot 2024-10-03 at 3 50 52 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

nice! I agree the table is the way to go 🎉

Copy link

codecov bot commented Oct 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.74%. Comparing base (da9100d) to head (791df25).
Report is 48 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2070   +/-   ##
=======================================
  Coverage   91.74%   91.74%           
=======================================
  Files          38       38           
  Lines       10081    10083    +2     
  Branches      631      631           
=======================================
+ Hits         9249     9251    +2     
  Misses        820      820           
  Partials       12       12           
Flag Coverage Δ
cli-hooks 95.23% <ø> (ø)
cli-test 95.69% <ø> (+<0.01%) ⬆️
oauth 77.39% <ø> (ø)
socket-mode 58.22% <ø> (ø)
web-api 96.89% <ø> (ø)
webhook 96.65% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

"start": "docusaurus start",
"build": "docusaurus build",
"start": "TYPEDOC_WATCH=true docusaurus start",
"build": "TYPEDOC_WATCH=false docusaurus build",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Followed these docs to set up proper TypeScript reference doc re-generation watching: https://typedoc-plugin-markdown.org/plugins/docusaurus/watch-mode

@@ -9,59 +9,59 @@ export enum LogLevel {
}

/**
* Interface for objects where objects in this package's logs can be sent (can be used as `logger` option).
* Logging interface exposing different methods to log at different {@link LogLevel}s.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By using {@link} inline tags, the generated reference documentation figures out what to link to 😍
Screenshot 2024-10-03 at 3 49 25 PM

@@ -94,53 +95,35 @@ export class ConsoleLogger implements Logger {
return this.level;
}

/**
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 can forego the docs here because docs generation is smart enough to pull in, as a fallback, the description of this method from the Logger interface this class implements!

Screenshot 2024-10-03 at 3 50 52 PM

@filmaj
Copy link
Contributor Author

filmaj commented Oct 3, 2024

Odd, docs building seems to fail in CI but runs fine for me locally 🤔 need to investigate more.

@filmaj filmaj marked this pull request as draft October 3, 2024 20:27
@@ -31,6 +31,22 @@ jobs:
run: npm ci
working-directory: ./docs

- name: Install sub-package dependencies
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the docs sidebar generation relies on importing and parsing TypeScript source of each sub-package we wish to document, we need to pull in the dependencies of each sub-package before building can begin.

@@ -32,10 +58,12 @@ const config = {
/** @type {import('@docusaurus/preset-classic').Options} */
({
docs: {
sidebarItemsGenerator: async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

out: "./content/reference/webhook",
}
]
...PACKAGES.map((pkg) => ['docusaurus-plugin-typedoc', packageReferenceOptions(pkg)]),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

DRYing things up here

docs/sidebars.js Outdated
type: 'doc',
id: `reference/${pkg}/index`,
},
items: await waitAndImportFile(`./content/reference/${pkg}/typedoc-sidebar.cjs`),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One issue I saw playing around with the docs is that sometimes, as part of docs re-generation, I would get MODULE_NOT_FOUND errors where the sidebar generation was executing, and the generated typedoc files for each sub-package was missing. I think this has something to do with the docusaurus lifecycle: plugins we use, like typedoc, are still executing will docusaurus is trying to generate the sidebar. This lifecycle is a bit unclear to me. In any case, to prevent this error, I rewrote this part so that requireing the relevant sub-package's generated typedoc only happens once the generated file is actually present. It seems to have addressed the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

is the file pretty much guaranteed to generate within 50ms or should we increase this for flexibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hard to say 😢 initially, at least when running locally, I had this set to 200 and things worked fine. After a while I decided to drop to 50 to see if that changed behaviour, but that also seems to work locally 🤷

'packages/rtm-api',
'packages/webhook',
'packages/socket-mode',
{ type: 'doc', id: 'getting-started' },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving to the more fine-control approach of providing our own asynchronous sidebar generator function, for some reason, requires the output of the function to be the more verbose list-of-objects for sidebar items - I'd get errors otherwise. So, had to move away from the the convenience string items 🤷

docs/sidebars.js Outdated
},
],
label: 'API Reference',
items: apiReferenceItems,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part was also DRYed up a bit

@@ -4,7 +4,7 @@
* work well for content-centric websites.
*/

:root {
:root {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Purely linter fixes

import Translate from '@docusaurus/Translate';
import Heading from '@theme/Heading';
export default function NotFoundContent({className}) {
import clsx from 'clsx';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Purely linter fixes

import React from 'react';
import {translate} from '@docusaurus/Translate';
import {PageMetadata} from '@docusaurus/theme-common';
import { translate } from '@docusaurus/Translate';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Linter fixes only here

@@ -1,20 +1,20 @@
/* Color variables */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Linter fixes only here

@filmaj filmaj marked this pull request as ready for review October 4, 2024 12:48
Copy link
Contributor

@hello-ashleyintech hello-ashleyintech left a comment

Choose a reason for hiding this comment

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

excellent work and LGTM 😄 left a few questions, mostly for my learning purposes 🧠

If we've led you astray, please let us know. We'll do our best to get things in order.

<Translate id="theme.NotFound.p1" description="The first paragraph of the 404 page">
If we've led you astray, please let us know. We'll do our best to get things in order.
Copy link
Contributor

Choose a reason for hiding this comment

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

this might be more of a copy nit for a follow up PR, but should we include some sort of link or CTA here so folks can leave feedback on the docs?

Copy link
Contributor Author

@filmaj filmaj Oct 4, 2024

Choose a reason for hiding this comment

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

Great call. @hello-ashleyintech perhaps you can help me, a total React noob, here: I tried wrapping the "please let us know" text in an <a href=""></a> tag and link to this repo's issue creation, but I get a, I think, React error 😢

How.. does one do links in React? 👶

Screenshot 2024-10-04 at 9 55 36 AM

Copy link
Contributor

@hello-ashleyintech hello-ashleyintech Oct 4, 2024

Choose a reason for hiding this comment

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

ooooh yes! That looks like it might actually be caused by embedding a link in the <Translate> tag from Docusaurus. It seems like that tag only supports simple strings. The suggested workaround in the docs is to break this into substrings wrapped in the <Translate> tag - maybe to keep that link as part of the translation it could be something like:

<Translate>If we've led you astray, </Translate><a href=""><Translate>please let us know</Translate></a><Translate>. We'll do our best to get things in order.</Translate>

If that doesn't work though then we might need to just not translate the please let us know line for now 😬

*/
function packageReferenceOptions(pkg) {
return {
classPropertiesFormat: 'table',
Copy link
Contributor

Choose a reason for hiding this comment

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

nice! I agree the table is the way to go 🎉

docs/sidebars.js Outdated
type: 'doc',
id: `reference/${pkg}/index`,
},
items: await waitAndImportFile(`./content/reference/${pkg}/typedoc-sidebar.cjs`),
Copy link
Contributor

Choose a reason for hiding this comment

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

is the file pretty much guaranteed to generate within 50ms or should we increase this for flexibility?

@lukegalbraithrussell
Copy link
Contributor

Closing for now -- we ran into build issues with this approach. I'll take another crack at this early next year

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:typescript issues that specifically impact using the package from typescript projects docs M-T: Documentation work only semver:patch tests M-T: Testing work only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants