Review requested:

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

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.72%. Comparing base (9cc7fcc) to head (95a3dc8).
⚠️ Report is 1 commits behind head on main.

@@           Coverage Diff           @@
##             main   #57343   +/-   ##
=======================================
  Coverage   89.72%   89.72%           
=======================================
  Files         675      675           
  Lines      204797   204797           
  Branches    39344    39353    +9     
=======================================
+ Hits       183752   183758    +6     
- Misses      13324    13329    +5     
+ Partials     7721     7710   -11     

Do you mean to say the "node:process's 'message' event params are no longer parsed correctly:" is an issue on the markdown or?

Yes. The new tooling is much stricter on what it considers valid vs invalid. The markdown for that particular event would be considered invalid by the new tooling.

Once this PR lands, we can land the new linter, which will identify invalid markdown before they land.

I'd argue we should fix that specific Markdown file before we land this PR, otherwise that section is broken.

See #61756

A few bugs I've found while browsing the JSON diff:

When a function has multiple signatures, on this PR it's no longer taken into account, the JSON no longer contains info about e.g. return type or history

REPLACEME is for some reason considered older instead of newer (FWIW we have a linter rule that enforces the order in the markdown is correct, so the markdown order should be used instead)

The tool identify as params things that clearly could not be valid JS identifiers

Also it looks like #57516 was never ported to doc-kit (i.e. having stability labels sticking to the top of the page), any reason for not doing it?

I guess the first item is potentially breaking, do we know how easy/hard is it to fix? I don't think it's a blocker for landing, it might be one for backporting though. The other ones are nice-to-have, and should be easy to fix anyway.

When a function has multiple signatures, on this PR it's no longer taken into account, the JSON no longer contains info about e.g. return type or history

It still does, however, it correctly places the return type, history, etc with the signature it is associated with. (i.e. you'll find all the meta items within the signature array.

is for some reason considered older instead of newer (FWIW we have a linter rule that enforces the order in the markdown is correct, so the markdown order should be used instead)

Noted. We can just remove the sortChanges function. However, REPLACEME changes are never included in the final release, so it shouldn't really matter

The tool identify as params things that clearly could not be valid JS identifiers

There's a give/take here. The more exclusive the parsing is, the higher the chance of a false-negative. The less exclusive, the higher the chance of a false positive. That being said, I don't think it'll break anything if we exclude [0-9'"] from starting the RegEx in the CAMEL_CASE constant.

It still does, however, it correctly places the return type, history, etc with the signature it is associated with. (i.e. you'll find all the meta items within the signature array.

There are two cases:

• each signature has its own doc (e.g. process.emitWarning):
  • ✅ current tooling attaches different metadata (history, return type, params) to each.
  • ✅ doc-kit attaches different metadata (history, return type, params) to each.
• both signatures share the same doc (e.g. stream.pipeline):
  • ✅ current tooling interprets it as a fall-through and attach same metadata to both.
  • ❌ doc-kit attaches no metadata to the first signature.

That's a regression, not a major one, but still one worth fixing before we can safely backport.

REPLACEME changes are never included in the final release, so it shouldn't really matter

It definitely matters for the DX though

It definitely matters for the DX though

It is called time travel, Antoine, never heard of? Future versions actually came before! 😛

• doc-kit attaches no metadata to the first signature.

@avivkeller are you working on a fix for this?

are you working on a fix for this?

It's a little more complex than you'd think. We currently handle parsing "entry by entry" so, one heading section is passed at a time. Passing multiple would require re-thinking the way we handleEntry in legacy-json.

are you working on a fix for this?

It's a little more complex than you'd think. We currently handle parsing "entry by entry" so, one heading section is passed at a time. Passing multiple would require re-thinking the way we handleEntry in legacy-json.

@aduh95 are we fine as is-then?

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

Looks like swc (our minifier), does not support all builds, causing most of the CI failures.

cc @ovflowd @flakey5

Looks like swc (our minifier), does not support all builds, causing most of the CI failures.

cc @ovflowd @flakey5

What minifier? The HTML one?

Yes. SWC's HTML minifier doesn't have bindings for some of the more niche machines. It has the following bindings:

        "@swc/html-darwin-arm64": "1.15.11",
        "@swc/html-darwin-x64": "1.15.11",
        "@swc/html-linux-arm-gnueabihf": "1.15.11",
        "@swc/html-linux-arm64-gnu": "1.15.11",
        "@swc/html-linux-arm64-musl": "1.15.11",
        "@swc/html-linux-x64-gnu": "1.15.11",
        "@swc/html-linux-x64-musl": "1.15.11",
        "@swc/html-win32-arm64-msvc": "1.15.11",
        "@swc/html-win32-ia32-msvc": "1.15.11",
        "@swc/html-win32-x64-msvc": "1.15.11"

@Renegade334 I removed a test you added 17h ago as this test becomes irrelevant with doc-kit (it tests for that already and that's also covered (or planned to be covered, if not yet) on the new linting package for @node-core's lint pkg)

de nada 🫡

this test becomes irrelevant with doc-kit (it tests for that already and that's also covered (or planned to be covered, if not yet) on the new linting package

That doesn't sound like a good reason to remove it, if it already passes why not keep it? If it doesn't, we should investigate why.

this test becomes irrelevant with doc-kit (it tests for that already and that's also covered (or planned to be covered, if not yet) on the new linting package

That doesn't sound like a good reason to remove it, if it already passes why not keep it? If it doesn't, we should investigate why.

It would have passed, the issue is that it relies on the old tooling internals, you can compare the imports. Also seems like René is fine with the removal.