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.

Sign up for GitHub

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

Jump to bottom

Replace ESDocs generated api-docs with api-documenter markdown #5163

Merged
merged 5 commits into from Jan 20, 2023
Merged

Replace ESDocs generated api-docs with api-documenter markdown #5163

merged 5 commits into from Jan 20, 2023

Conversation

robwalch
Copy link
Collaborator

@robwalch robwalch commented Jan 16, 2023 •
edited

This PR will...

Replace esdocs generated api-docs with api-documenter to markdown-styles static html.

Why is this Pull Request needed?

Cleanup documentation to reflect project interfaces and types. The ESDoc generated docs do not reflect the working structure of the project or its TypeScript definitions.

This change will allow for better integration between the tailored docs/API.md guide and generated API docs created from TypeScript and doc comments already being processed by api-extractor. For more see https://api-extractor.com/pages/setup/generating_docs/

@robwalch robwalch added this to the 1.4.0 milestone Jan 16, 2023
@robwalch robwalch changed the title Replace esdocs generated api-docs with api-documenter markdown Replace ESDocs generated api-docs with api-documenter markdown Jan 16, 2023
@robwalch robwalch force-pushed the feature/variable-substitution branch from 53a339c to b8a9635 Compare January 16, 2023 05:58
@robwalch robwalch force-pushed the task/api-docs-replace-esdocs-with-api-documenter-markdown branch from fed374e to 0aa44f6 Compare January 16, 2023 06:44
@tjenkinson
Copy link
Member

Looks good but I'm not sure about checking the generated markdown into the repo. There's currently nothing to validate that the docs are actually up to date so it's easy for them to go out of sync.

How about we keep them in the gitignore and build them on ci, and run the md files through a html generator for netlify if netlify doesn't support serving md as html automatically (just looking into that now)?

@tjenkinson
Copy link
Member

@robwalch I pushed 93244c5 to a branch which adds some validation to fail if the docs aren't in sync. I think with that it should be fine in the repo itself and less complicated than getting it built to html for netlify

With that LGTM!

tjenkinson
tjenkinson reviewed Jan 16, 2023
View reviewed changes
scripts/build-netlify.sh Outdated
@@ -10,8 +10,12 @@ echo "Building netlify..."

# redirect / to /demo
echo "/ /demo" > "$root/_redirects"

# rediect /api-docs to github API.md
echo "/api-docs https://github.com/video-dev/hls.js/tree/master/docs/API.md" > "$root/_redirects"
Copy link
Member

Choose a reason for hiding this comment

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

Just had a thought. It would be good to redirect to the commit specific url here it master so that old versions go to their correct docs

Copy link
Member

Choose a reason for hiding this comment

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

Can get the commit same way we do here

hls.js/scripts/deploy-netlify.sh

Line 6 in 0aa44f6

currentCommit=$(git rev-parse HEAD)

which should work when this runs in Netlify or actions

Copy link
Collaborator Author

@robwalch robwalch Jan 17, 2023 •
edited

Choose a reason for hiding this comment

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

Can get the commit same way we do here

Hmm. Would be nice to see the version in the docs under Hls.version (but docs are generated from src not compiled js).

Looks good but I'm not sure about checking the generated markdown into the repo. There's currently nothing to validate that the docs are actually up to date so it's easy for them to go out of sync.

How about we keep them in the gitignore and build them on ci, and run the md files through a html generator for netlify if netlify doesn't support serving md as html automatically (just looking into that now)?

I would like to do this. Got it working with marked but the output is unstyled. Still looking into it.

I pushed 93244c5 to a branch which adds some validation to fail if the docs aren't in sync. I think with that it should be fine in the repo itself and less complicated than getting it built to html for netlify

Thanks for this. It is less complicated, but could lead to some confusion. Still looking into it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

markdown-styles works really nicely. After #5161 is in I'll rebased and check the netlify preveiw.

Copy link
Member

Choose a reason for hiding this comment

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

Yeh looks great

@robwalch robwalch force-pushed the task/api-docs-replace-esdocs-with-api-documenter-markdown branch 3 times, most recently from db9316d to a1725b8 Compare January 17, 2023 04:49
robwalch
robwalch commented Jan 17, 2023
View reviewed changes
package.json
@@ -27,15 +27,16 @@
"build:watch": "webpack --progress --env debug --env demo --watch",
"build:types": "tsc --build tsconfig-lib.json && api-extractor run --local",
"dev": "webpack serve --progress --env debug --env demo --port 8000 --static .",
"docs": "esdoc",
"docs": "doctoc ./docs/API.md && api-documenter markdown -i api-extractor -o api-extractor/api-documenter && npm run docs-md-to-html",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since doctoc ./docs/API.md modifies API.md should we still run a git diff --exit-code check on it?

Copy link
Member

Choose a reason for hiding this comment

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

Yeh I think it would be good to still do the check for api.md to make sure the contents is up to date

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cherry-picked 93244c5 and removed the part that checked api-docs as that is no longer checked in as part of these changes.

@itsjamie
Copy link
Collaborator

Thanks! I'm glad I can archive those repos now 👍

itsjamie
itsjamie previously approved these changes Jan 17, 2023
View reviewed changes
@robwalch robwalch force-pushed the feature/variable-substitution branch 2 times, most recently from a0d75e7 to f1996ef Compare January 18, 2023 22:55
@robwalch robwalch force-pushed the task/api-docs-replace-esdocs-with-api-documenter-markdown branch from a1725b8 to 4f8d3ae Compare January 18, 2023 23:12
Base automatically changed from feature/variable-substitution to master January 20, 2023 00:52
@robwalch robwalch force-pushed the task/api-docs-replace-esdocs-with-api-documenter-markdown branch from 4f8d3ae to f25fa56 Compare January 20, 2023 01:18
tjenkinson
tjenkinson previously approved these changes Jan 20, 2023
View reviewed changes
Copy link
Member

@tjenkinson tjenkinson left a comment

Choose a reason for hiding this comment

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

Lgtm

Only comment is we should fix the versions in the package.json

package.json Outdated
"@itsjamie/esdoc-ecmascript-proposal-plugin": "0.5.0",
"@itsjamie/esdoc-standard-plugin": "0.5.0",
"@itsjamie/esdoc-typescript-plugin": "0.5.0",
"@microsoft/api-documenter": "^7.19.27",
Copy link
Member

Choose a reason for hiding this comment

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

These should be fixed versions and then renovate work bump and test with new versions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@robwalch robwalch merged commit 485fe15 into master Jan 20, 2023
@robwalch robwalch deleted the task/api-docs-replace-esdocs-with-api-documenter-markdown branch January 20, 2023 16:57
robwalch added a commit that referenced this pull request Jan 26, 2023
@robwalch
Replace ESDocs generated api-docs with api-documenter markdown (#5163)
8740846
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tjenkinson tjenkinson tjenkinson approved these changes

@itsjamie itsjamie itsjamie left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.4.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@robwalch @tjenkinson @itsjamie