build, doc: use new api doc tooling

[flakey5]

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]

Review requested:

• [ ] @nodejs/nodejs-website
• [ ] @nodejs/web-infra

[flakey5]

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]

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

[ovflowd]

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[bot]]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 88.52%. Comparing base (cbe0233) to head (d1ba077).

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57343      +/-   ##
==========================================
- Coverage   88.53%   88.52%   -0.01%     
==========================================
  Files         703      703              
  Lines      208413   208413              
  Branches    40191    40191              
==========================================
- Hits       184521   184505      -16     
- Misses      15902    15926      +24     
+ Partials     7990     7982       -8     

see 41 files with indirect coverage changes

:rocket: New features to boost your workflow:

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

[araujogui]

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]

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]

@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 (?)

[ovflowd]

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

[ovflowd]

cc @jasnell would you like to review?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/65723/

[mhdawson]

Was about to take a look, but I see many failed ci jobs?

[ovflowd]

I wonder if the failures are even related?

[flakey5]

I think they are related, it can't find a node binary when running npm install in tools/doc. Will take a look in a bit

[Trott]

I think they are related, it can't find a node binary when running npm install in tools/doc. Will take a look in a bit

I'm not looking closely, but generally, we run deps/npm/bin/npm-cli.js instead of npm for this reason. In Makefile, there's an NPM constant defined for that purpose.

(Apologies in advance if I'm totally off base and this is useless noise.)

[ovflowd]

I think they are related, it can't find a node binary when running npm install in tools/doc. Will take a look in a bit

I'm not looking closely, but generally, we run deps/npm/bin/npm-cli.js instead of npm for this reason. In Makefile, there's an NPM constant defined for that purpose.

(Apologies in advance if I'm totally off base and this is useless noise.)

Yup! We are doing that already afaik! But thanks for the insights here 🫶

[ovflowd]

I tried to run make docserve -j on this branch and got the following error:

Error: Command failed: git remote get-url origin
error: No such remote 'origin'

@flakey5 any comment here? @aduh95 afaik there is no built-in web server for the new tooling. Even more as they are just static HTML files you can simply open them on the browser?

What is your suggestion for the behaviour to be? Should docserve have a similar behaviour as webpack-devserver? It builds the api doc tooling and keeps watching for changes and rebuilds?

cc @flakey5 can we maybe add a --serve CLI argument to the tooling, using Node.js built-in HTTP server to serve these files? And maybe a --watch argument for watching for changes and rebuilding on demand? (Both arguments should be independent, but can work together, aka, webserver will simply server the built files and keep the server running until a kill signal is sent, same for --watch, it will keep watching the FS and do changes when detected? We can open issues for that, I do believe these are a blocker.

[ovflowd]

I tried to run make docserve -j on this branch and got the following error:

Error: Command failed: git remote get-url origin
error: No such remote 'origin'

@flakey5 any comment here? @aduh95 afaik there is no built-in web server for the new tooling. Even more as they are just static HTML files you can simply open them on the browser?

What is your suggestion for the behaviour to be? Should docserve have a similar behaviour as webpack-devserver? It builds the api doc tooling and keeps watching for changes and rebuilds?

cc @flakey5 can we maybe add a --serve CLI argument to the tooling, using Node.js built-in HTTP server to serve these files? And maybe a --watch argument for watching for changes and rebuilding on demand? (Both arguments should be independent, but can work together, aka, webserver will simply server the built files and keep the server running until a kill signal is sent, same for --watch, it will keep watching the FS and do changes when detected? We can open issues for that, I do believe these are a blocker.

Or is this related to the git issue you were referring, @aduh95?

[ovflowd]

I tried to run make docserve -j on this branch and got the following error:

Error: Command failed: git remote get-url origin
error: No such remote 'origin'

@flakey5 any comment here? @aduh95 afaik there is no built-in web server for the new tooling. Even more as they are just static HTML files you can simply open them on the browser? What is your suggestion for the behaviour to be? Should docserve have a similar behaviour as webpack-devserver? It builds the api doc tooling and keeps watching for changes and rebuilds? cc @flakey5 can we maybe add a --serve CLI argument to the tooling, using Node.js built-in HTTP server to serve these files? And maybe a --watch argument for watching for changes and rebuilding on demand? (Both arguments should be independent, but can work together, aka, webserver will simply server the built files and keep the server running until a kill signal is sent, same for --watch, it will keep watching the FS and do changes when detected? We can open issues for that, I do believe these are a blocker.

Or is this related to the git issue you were referring, @aduh95?

Bump, @aduh95

[ovflowd]

I've made a PR to fix the current failures: https://github.com/nodejs/api-docs-tooling/pull/231

[avivkeller]

Hey everyone,

The Website and Web-Infra teams have been looking into an issue where GitHub Actions seemed to report incorrect line numbers for REPLACEME (and other invalid versions) in the linter. At first, it looked like the errors weren’t lining up, and even files without REPLACEME comments were flagged. But after digging in, we realized the issue wasn’t with the reporting.

Since Actions use a merge commit, the reported line numbers were actually correct in the main branch. The mismatch happened because the PR’s head branch was outdated—it didn’t have that merge commit, so it had older versions of the files. That made it look like the errors were in the wrong place. Once we updated the branch, everything lined up correctly, and the errors disappeared. That also explains why we couldn’t reproduce it locally—our branches didn’t include the merge commit.

For more information on our investigation into this, please see this Slack thread.

In short: everything was working fine; the GitHub UI's use of the outdated branch when showing flagged files just made it seem like it wasn’t.

[aduh95]

In short: everything was working fine

How is it working fine if it reports REPLACEME as an invalid version?

[ovflowd]

In short: everything was working fine

How is it working fine if it reports REPLACEME as an invalid version?

Apparently GitHub shows the file contents from files on the parent branch. So if the PR branch is out of sync with main, the api-docs-tooling is correctly reporting the files, but the GitHub PR diff is showing the files from main (Maybe my words are not explaining it correctly, @avivkeller can you explain better?)

[ovflowd]

The branch got rebased and it seems to be correctly saying which YAML blocks have errors. Although, cc @araujogui it'd be good if it shown the exact line within the YAML.

[aduh95]

I mean, not showing the correct line is indeed something we should try to fix (but IMO not blocking). However, I think a more problematic behavior is to report REPLACEME as an invalid version, I think that should be blocking as it will nudge contributors to not follow our process. (also it doesn't seem to treat an invalid version as hard failure, which is a regression)

[avivkeller]

How is it working fine if it reports REPLACEME as an invalid version?

Currently, we have it setup to report REPLACEME as an invalid version. While this behavior can be modified in the future, I don't is consider it an issue with the linter at the moment because it is a deliberate decision.

Apparently GitHub shows the file contents from files on the parent branch. So if the PR branch is out of sync with main, the api-docs-tooling is correctly reporting the files, but the GitHub PR diff is showing the files from main (Maybe my words are not explaining it correctly, @avivkeller can you explain better?)

GitHub's behavior during a pull request (PR) process can cause discrepancies between what api-docs-tooling reports and what the GitHub PR diff displays.

Here’s what happens:

• On the pull_request Actions trigger, github.ref is set to refs/pull/PULL_REQUEST_NUMBER/merge.
• When actions/checkout retrieves this ref, it checks out this version of the repository, which is already rebased with main.
• As a result, api-docs-tooling correctly reports linting errors from the rebased version (which includes changes from main).

However, the GitHub PR diff operates differently:

• It takes the information from the merged version (the rebase with main) but applies it to the PR head ref (the version of the branch before the merge).
• This discrepancy causes the confusion because the PR diff shows the flagged data from main on the PR head ref's files.

[ovflowd]

I mean, not showing the correct line is indeed something we should try to fix (but IMO not blocking). However, I think a more problematic behavior is to report REPLACEME as an invalid version, I think that should be blocking as it will nudge contributors to not follow our process. (also it doesn't seem to treat an invalid version as hard failure, which is a regression)

We treat REPLACEME as a warning, not an error. More to create a nudge to remind contributors to eventually change the REPLACEME to a version.

[avivkeller]

Tests are failing due to https://github.com/nodejs/api-docs-tooling/pull/228, however this function should be exported from node:fs in the generated node executable. I can revert the PR if needed, but I don't think this error should've occured:(https://github.com/nodejs/node/actions/runs/14044808144/job/39323129311?pr=57343#step:11:5337

Edit: Reverting @ https://github.com/nodejs/api-docs-tooling/pull/237

[ovflowd]

Tests are failing due to nodejs/api-docs-tooling#228, however this function should be exported from node:fs in the generated node executable. I can revert the PR if needed, but I don't think this error should've occured:(https://github.com/nodejs/node/actions/runs/14044808144/job/39323129311?pr=57343#step:11:5337

The CI seems to be using Node v20, not sure if intentional.

[ovflowd]

But FWIW @avivkeller let’s not use experimental features on the api doc tooling. Add the glob package back and lets just stay on the safe side :)