Review requested:

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

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

Also not sure what's going on with lint-addon-docs? cc @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)?

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 Report

All modified and coverable lines are covered by tests ✅

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

@@            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     

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.

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

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

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

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

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

Odd, it should be getting the version from the process variable. Running locally on main it gives me 24.0.0 (or something similar, gotta double check)

Probably b/c you are using the built node executable, whereas this is building from v22.14.0 (Github Actions uses the latest LTS).

I recall there was a request from someone that we reported on that so that no one forgets to accidentally make a release without replacing all REPLACEME.

There are already checks for that, added in #24575:

make: *** No rule to make target 'out/doc/api/index.html'

Where was this seen?

• We should not report REPLACEME as an invalid version.

• We should report invalid versions as a hard failure.

This has been addressed :-)

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

This can probably be solved by explicitly passing the version of Node.js we are trying to generate to the new tooling. IIRC there should be some parsing logic in the Makefile. (I think it's

make: *** No rule to make target 'out/doc/api/index.html'

Where was this seen?

If you run make out/doc/api/index.html from the root of the repo, it works without this PR, it fails with it. The actual issue being that if we were to merge this, https://nodejs.org/api/index.html would be 404.

make: *** No rule to make target 'out/doc/api/index.html'

Where was this seen?

If you run make out/doc/api/index.html from the root of the repo, it works without this PR, it fails with it. The actual issue being that if we were to merge this, https://nodejs.org/api/index.html would be 404.

@flakey5 have you restored index.md?

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

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 👍

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

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

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? 🙏

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.

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

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.

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

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