build, doc: use new api doc tooling #57343

flakey5 · 2025-03-06T06:23:57Z

Switches over to using the new doc generation tooling. For more background on this, please see #52343

~~Currently a draft just to get feedback on the approach to this integration.~~

cc @nodejs/web-infra

nodejs-github-bot · 2025-03-06T06:24:02Z

Review requested:

@nodejs/nodejs-website
@nodejs/web-infra

Makefile

vcbuild.bat

tools/doc/README.md

ovflowd

@flakey5 I guess you also need to check our GitHub Action Workflows, and also other mentions of these files within the source.

Like within the Contributor Docs, there is a file that describes how the legacy API doc tooling works, and I believe there are other references also.

flakey5 · 2025-03-10T22:21:09Z

lint-js-and-md is failing because of the linting errors since it exits with a non-zero status code if there's anything wrong with the docs. I think we should skip the REPLACEME checks for normal ci runs.

It also looks like synopsis.md fails the introduced_in check because it's not under the top-level header, should it be enforced that introduced_in goes under the top-level header or should we change it to just check that it exists somewhere in the file (like it was doing previously)?

flakey5 · 2025-03-10T22:34:35Z

Also not sure what's going on with lint-addon-docs? cc @araujogui

ovflowd · 2025-03-10T22:38:04Z

lint-js-and-md is failing because of the linting errors since it exits with a non-zero status code if there's anything wrong with the docs. I think we should skip the REPLACEME checks for normal ci runs.

It also looks like synopsis.md fails the introduced_in check because it's not under the top-level header, should it be enforced that introduced_in goes under the top-level header or should we change it to just check that it exists somewhere in the file (like it was doing previously)?

REPLACEME shouldn't error, imo, just give a warning. Our linter should have warn and error levels.

And yes introduced_in must be top level!

codecov · 2025-03-10T23:53:32Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.19%. Comparing base (422529a) to head (8b72e5a).

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57343      +/-   ##
==========================================
- Coverage   90.27%   90.19%   -0.09%     
==========================================
  Files         630      628       -2     
  Lines      186124   185689     -435     
  Branches    36469    36339     -130     
==========================================
- Hits       168021   167475     -546     
- Misses      10974    11140     +166     
+ Partials     7129     7074      -55

see 49 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Makefile

araujogui · 2025-03-11T01:49:00Z

lint-js-and-md is failing because of the linting errors since it exits with a non-zero status code if there's anything wrong with the docs. I think we should skip the REPLACEME checks for normal ci runs.

It also looks like synopsis.md fails the introduced_in check because it's not under the top-level header, should it be enforced that introduced_in goes under the top-level header or should we change it to just check that it exists somewhere in the file (like it was doing previously)?

Actually, the linter only returns 1 if there's an error-level issue, and I don't think that's the case here.

araujogui · 2025-03-11T01:49:50Z

Also not sure what's going on with lint-addon-docs? cc @araujogui

I’m not sure either, but I’ll check it out.

ovflowd · 2025-03-12T10:26:58Z

@flakey5:

3:1-3:9   warning Use "the Node.js" instead of "Node.js'" prohibited-strings remark-lint
4:46-4:50 warning Use "Node.js" instead of "Node"         prohibited-strings remark-lint

On the README.md file you updated (tools/doc/README.md) after updating those can you run make format-md (?)

Makefile

ovflowd

This is the result of many months of arduous work between many awesome folks, including @flakey5 @AugustinMauroy @araujogui @ovflowd @avivkeller and others.

I'm so proud of what we are achieving here and this is a huge step towards a modern tooling and a revamped API docs within Node.js

Approving, as I believe this is ready!

ovflowd · 2025-03-13T17:24:16Z

cc @nodejs/collaborators can we have another approval here? 🙏

lpinca

RSLGTM because it is hard to review and outside of my comfort zone.

flakey5 · 2025-04-06T01:25:28Z

The actual issue being that if we were to merge this, https://nodejs.org/api/index.html would be 404.

That shouldn't be the case? The index.html file is still generated w/ make doc-only and make doc

Makefile

aduh95 · 2025-04-06T09:55:48Z

The actual issue being that if we were to merge this, https://nodejs.org/api/index.html would be 404.

That shouldn't be the case? The index.html file is still generated w/ make doc-only and make doc

I can confirm that out/doc/api/index.html is correctly generated now 👍

avivkeller · 2025-04-06T17:35:47Z

@aduh95 NODE_RELEASED_VERSIONS is missing v0.11.15 and v0.1.96, causing the linter to fail, what do you recommend? Hardcoding?

aduh95 · 2025-04-06T22:41:23Z

Here's what we currently have: https://github.com/nodejs/remark-preset-lint-node/blob/79792fc74f48f467b8c649672c08003a55bba4ed/remark-lint-nodejs-yaml-comments.js#L55-L60
It looks like it's incorrectly ignoring v0.11.x as well, we should look into that

ovflowd · 2025-04-11T20:06:13Z

Here's what we currently have: https://github.com/nodejs/remark-preset-lint-node/blob/79792fc74f48f467b8c649672c08003a55bba4ed/remark-lint-nodejs-yaml-comments.js#L55-L60 It looks like it's incorrectly ignoring v0.11.x as well, we should look into that

We just fixed it upstream. @flakey5 can you update the version used here? And rebase the PR so that @aduh95 can remove the block and we can finally merge this? 🙏

aduh95

Issues that need to be addressed:

We should keep the existing tests (at least for now, can be removed in a follow up).
~~We should not report REPLACEME as an invalid version.~~ addressed, thank you!
~~We should report invalid versions as a hard failure.~~ addressed, thank you!
It pretends it generates the docs for Node.js 22.14.0 instead of 24.0.0 🤨
~~make: *** No rule to make target 'out/doc/api/index.html'~~

I would note that it seems there no cache mecanism, running make docserve takes forever on my quite powerful machine, even if I've made no changes to the files since the last doc build. Just a complain, not a blocker.

Makefile

ovflowd · 2025-04-20T20:24:01Z

Hey @avivkeller, I noticed that you pushed a commit to @flakey5's pull request. It’s generally not common practice for contributors to push changes to someone else's pull request on the nodejs/node repository without explicit permission from the author. Please keep that in mind! Additionally, since you’re not a core collaborator, you shouldn’t perform actions that are reserved for core collaborators, which I believe this would qualify as.

If I’m wrong or if anyone has a different perspective, please feel free to correct me.

avivkeller · 2025-04-20T20:24:40Z

I don't have direct push permissions, even if I wanted to (I have read access to this repository). I created a pull request into https://github.com/flakey5/node that @flakey5 merged. While I authored the commit, I didn't push it to someone else's pull request.

See flakey5#1

avivkeller · 2025-04-20T20:27:56Z

It pretends it generates the docs for Node.js 22.14.0 instead of 24.0.0 🤨

Now, --version will explicitly specify the version to generate based on the $(VERSION) Makefile variable. See flakey5#1

We should keep the existing tests (at least for now, can be removed in a follow up).

Why? The files they are testing no longer exist. There's no point in testing files that aren't present or don't serve any function. Instead, we should incorporate any missing tests into the new tooling, rather than leaving them here to be ineffective.

flakey5 · 2025-04-20T20:57:24Z

I noticed that you pushed a commit to @flakey5's pull request

It was a pr that I merged.

We should keep the existing tests (at least for now, can be removed in a follow up).

I've re-added the tests that are still applicable. Note however that some of the tests need to be removed because the new tooling doesn't expose the same things.

Also noteworthy that we have tests for these things in the api-docs-tooling repo

Switches over to using the new doc generation tooling. For more background on this, please see nodejs#52343 Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> Co-authored-by: Claudio W <cwunder@gnome.org> Co-authored-by: avivkeller <me@aviv.sh>

ovflowd · 2025-04-20T21:11:37Z

I don't have direct push permissions, even if I wanted to (I have read access to this repository). I created a pull request into https://github.com/flakey5/node that @flakey5 merged. While I authored the commit, I didn't push it to someone else's pull request.

See flakey5#1

Noted, that makes more sense. For me it looked like @flakey5 rebased his PR; But indeed it was a PR they merged; Disregard my comment :D

Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>

avivkeller · 2025-05-03T18:25:26Z

@aduh95 could you re-review when you get a chance? I believe we have resolved all your concerns 😄

trivikr · 2025-05-04T16:04:34Z

Is there still time to get the new API docs tooling before 24.0.0 release on Tue, May 6th?

AugustinMauroy · 2025-05-04T19:33:44Z

Is there still time to get the new API docs tooling before 24.0.0 release on Tue, May 6th?

Wait for Antoine and fix CI. IDK Why it's fail

aduh95

I think there's still a problem in the Makefile, if I run rm -f out/doc/api/index.html && make out/doc/api/index.html, it errors with make: *** No rule to make target 'out/doc/api/index.html'. Stop.. This does not happen on main.

I'd also like to get some comment on the performance, here's the command I'm using to measure it:

make docclean doc-only -j && time make doc-only -j

On this PR: make doc-only -j 67.66s user 2.38s system 126% cpu 55.554 total
On main: make doc-only -j 1.23s user 0.33s system 118% cpu 1.317 total

aduh95 · 2025-05-04T19:53:57Z

tools/doc/update.sh

+
+rm -rf node_modules/ package-lock.json
+
+npm install "git://github.com/nodejs/api-docs-tooling#$latest_commit"


This should not rely on a globally installed npm and instead target the one in the tree, for an example see

node/tools/dep_updaters/update-acorn-walk.sh

Lines 10 to 13 in 995ad2b

BASE_DIR=$(cd "$(dirname "$0")/../.." && pwd)

[ -z "$NODE" ] && NODE="$BASE_DIR/out/Release/node"

[ -x "$NODE" ] || NODE=$(command -v node)

NPM="$BASE_DIR/deps/npm/bin/npm-cli.js"

Suggested change

npm install "git://github.com/nodejs/api-docs-tooling#$latest_commit"

"$NODE" "$NPM" install "git://github.com/nodejs/api-docs-tooling#$latest_commit"

aduh95 · 2025-05-04T19:54:33Z

tools/doc/update.sh

+CURL=$(command -v curl)
+
+if ! [ -x "$CURL" ]; then
+    echo curl is needed to update api doc tooling
+    exit 1
+fi
+
+latest_commit=$($CURL -s -H "Accept: application/vnd.github.VERSION.sha" "https://api.github.com/repos/nodejs/api-docs-tooling/commits/main")


nit: we could use node -e 'fetch(… to remove the dependency on curl

aduh95 · 2025-05-04T19:56:34Z

test/doctool/test-doctool-html.mjs

Could we have a comment on why we are removing this file? Sorry if it has already been answered, those long GH thread are hard to follow

aduh95 · 2025-05-04T19:58:03Z

test/doctool/test-doctool-json.mjs

Same here, could we have a comment on why we are removing this file?

aduh95 · 2025-05-04T19:58:10Z

test/doctool/test-doctool-versions.mjs

Same here, could we have a comment on why we are removing this file?

aduh95 · 2025-05-04T19:58:14Z

test/doctool/test-local-md-file-reference-regex.mjs

Same here, could we have a comment on why we are removing this file?

aduh95 · 2025-05-04T20:00:36Z

.github/workflows/build-tarball.yml

+      - name: Install Node.js
+        uses: actions/setup-node@1d0ff469b7ec7b3cb9d8673fde0c81c44821de2a  # v4.2.0
+        with:
+          node-version: ${{ env.NODE_VERSION }}


Let's remove this, we should be able to build node without node (it's probably related to #57343 (comment), no?)

aduh95 · 2025-05-04T21:00:09Z

Is there still time to get the new API docs tooling before 24.0.0 release on Tue, May 6th?

Even if it lands before then, it's unlikely to be included. Why do you ask?

trivikr · 2025-05-05T00:55:48Z

Even if it lands before then, it's unlikely to be included. Why do you ask?

Usually major changes are aligned with major version release, so wanted to know since we're close to 24.0.0 release.
The new docs tooling is going to introduced llms.txt requested in nodejs/nodejs.org#7723.

We'll have to wait for 24.1.0 release right?

avivkeller · 2025-05-05T01:04:41Z

Well, not really. This just includes the 1:1 HTML generation. The LLM file would be in a follow up if anything

avivkeller · 2025-05-05T12:07:52Z

On this PR: make doc-only -j 67.66s user 2.38s system 126% cpu 55.554 total On main: make doc-only -j 1.23s user 0.33s system 118% cpu 1.317 total

That's because, (like you said prior) in the original makefile, everything is built seperately, so individual files aren't rebuilt if they haven't changed:

out/doc/api/%.json out/doc/api/%.html: doc/api/%.md tools/doc/generate.mjs \
	tools/doc/markdown.mjs tools/doc/html.mjs tools/doc/json.mjs \
	tools/doc/apilinks.mjs $(VERSIONS_DATA) | $(LINK_DATA) out/doc/api
	@if [ "$(shell $(node_use_icu))" != "true" ]; then \
		echo "Skipping documentation generation (no ICU)"; \
	else \
		$(call available-node, $(gen-api)) \
	fi

Whereas, in this PR, they are all built, every time.

out/doc/api/%.json out/doc/api/%.html: doc-only

aduh95 · 2025-05-05T12:40:45Z

@avivkeller I can see that, it's in the diff; what's not in the diff is why is it like that, how to fix it, etc.

nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform. labels Mar 6, 2025

flakey5 marked this pull request as draft March 6, 2025 06:24

flakey5 force-pushed the flakey5/20250305/api-docs-tooling branch from 77ede22 to 3423c21 Compare March 6, 2025 06:29

flakey5 commented Mar 6, 2025

View reviewed changes

Makefile Outdated Show resolved Hide resolved

flakey5 commented Mar 6, 2025

View reviewed changes

vcbuild.bat Outdated Show resolved Hide resolved

flakey5 force-pushed the flakey5/20250305/api-docs-tooling branch from 3423c21 to 451f8a7 Compare March 6, 2025 06:31

flakey5 commented Mar 6, 2025

View reviewed changes

tools/doc/README.md Show resolved Hide resolved

ovflowd reviewed Mar 6, 2025

View reviewed changes

flakey5 force-pushed the flakey5/20250305/api-docs-tooling branch 3 times, most recently from cf2609b to a3ce99d Compare March 10, 2025 22:04

flakey5 marked this pull request as ready for review March 10, 2025 22:05

araujogui reviewed Mar 11, 2025

View reviewed changes

Makefile Outdated Show resolved Hide resolved

araujogui mentioned this pull request Mar 11, 2025

fix(addon-verify): await files writing nodejs/api-docs-tooling#207

Merged

2 tasks

ovflowd reviewed Mar 13, 2025

View reviewed changes

Makefile Outdated Show resolved Hide resolved

ovflowd approved these changes Mar 13, 2025

View reviewed changes

lpinca approved these changes Mar 13, 2025

View reviewed changes

aduh95 reviewed Apr 6, 2025

View reviewed changes

Makefile Show resolved Hide resolved

aduh95 mentioned this pull request Apr 6, 2025

doc: add missing v0.11.15 changelog entry #57779

Merged

avivkeller mentioned this pull request Apr 6, 2025

fix(lint): hardcode ignored invalid change versions nodejs/api-docs-tooling#246

Merged

bjohansebas mentioned this pull request Apr 12, 2025

doc: increase z-index of header element #57851

Closed

flakey5 force-pushed the flakey5/20250305/api-docs-tooling branch from 7df263f to a418c13 Compare April 12, 2025 22:40

aduh95 requested changes Apr 12, 2025

View reviewed changes

avivkeller reviewed Apr 12, 2025

View reviewed changes

Makefile Outdated Show resolved Hide resolved

flakey5 force-pushed the flakey5/20250305/api-docs-tooling branch from ddaa32a to 279e51f Compare April 20, 2025 21:03

fix ci

8b72e5a

Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>

aymen94 approved these changes Apr 24, 2025

View reviewed changes

aduh95 reviewed May 4, 2025

View reviewed changes


		rm -rf node_modules/ package-lock.json

		npm install "git://github.com/nodejs/api-docs-tooling#$latest_commit"

	BASE_DIR=$(cd "$(dirname "$0")/../.." && pwd)
	[ -z "$NODE" ] && NODE="$BASE_DIR/out/Release/node"
	[ -x "$NODE" ] \|\| NODE=$(command -v node)
	NPM="$BASE_DIR/deps/npm/bin/npm-cli.js"

	npm install "git://github.com/nodejs/api-docs-tooling#$latest_commit"
	"$NODE" "$NPM" install "git://github.com/nodejs/api-docs-tooling#$latest_commit"

build, doc: use new api doc tooling #57343

Are you sure you want to change the base?

build, doc: use new api doc tooling #57343

Conversation

flakey5 commented Mar 6, 2025 • edited Loading

nodejs-github-bot commented Mar 6, 2025

ovflowd left a comment

Choose a reason for hiding this comment

flakey5 commented Mar 10, 2025 • edited Loading

flakey5 commented Mar 10, 2025

ovflowd commented Mar 10, 2025

codecov bot commented Mar 10, 2025 • edited Loading

Codecov Report

araujogui commented Mar 11, 2025

araujogui commented Mar 11, 2025

ovflowd commented Mar 12, 2025

ovflowd left a comment

Choose a reason for hiding this comment

ovflowd commented Mar 13, 2025

lpinca left a comment • edited Loading

Choose a reason for hiding this comment

flakey5 commented Apr 6, 2025

aduh95 commented Apr 6, 2025

avivkeller commented Apr 6, 2025 • edited Loading

aduh95 commented Apr 6, 2025

ovflowd commented Apr 11, 2025

aduh95 left a comment • edited Loading

Choose a reason for hiding this comment

ovflowd commented Apr 20, 2025 • edited Loading

avivkeller commented Apr 20, 2025 • edited Loading

avivkeller commented Apr 20, 2025

flakey5 commented Apr 20, 2025

ovflowd commented Apr 20, 2025

avivkeller commented May 3, 2025

trivikr commented May 4, 2025

AugustinMauroy commented May 4, 2025

aduh95 left a comment

Choose a reason for hiding this comment

aduh95 May 4, 2025

Choose a reason for hiding this comment

aduh95 May 4, 2025

Choose a reason for hiding this comment

aduh95 May 4, 2025

Choose a reason for hiding this comment

aduh95 May 4, 2025

Choose a reason for hiding this comment

aduh95 May 4, 2025

Choose a reason for hiding this comment

aduh95 May 4, 2025

Choose a reason for hiding this comment

aduh95 May 4, 2025 • edited Loading

Choose a reason for hiding this comment

aduh95 commented May 4, 2025

trivikr commented May 5, 2025

avivkeller commented May 5, 2025

avivkeller commented May 5, 2025

aduh95 commented May 5, 2025

flakey5 commented Mar 6, 2025 •

edited

Loading

flakey5 commented Mar 10, 2025 •

edited

Loading

codecov bot commented Mar 10, 2025 •

edited

Loading

lpinca left a comment •

edited

Loading

avivkeller commented Apr 6, 2025 •

edited

Loading

aduh95 left a comment •

edited

Loading

ovflowd commented Apr 20, 2025 •

edited

Loading

avivkeller commented Apr 20, 2025 •

edited

Loading

aduh95 May 4, 2025 •

edited

Loading