Repository navigation
doc: move to redesign - #66713
doc: move to redesign#66713avivkeller wants to merge 4 commits into
Conversation
|
Review requested:
|
nodejs-github-bot
added
build
bmuenzenmeyer
left a comment
bmuenzenmeyer
left a comment
There was a problem hiding this comment.
I think we need to do better, align on fallback strategy here, or at least affirm that #66613 or this test is enough (not both) . If canBundle ever returns false, we don't get HTML docs at all, which we identified as a production issue worthy of an expensive new release.
Could we add this check to the release process too? Is CI enough? Perhaps
| const mdURL = new URL('../../doc/api/', import.meta.url); | ||
|
|
||
| // Kept in sync with `skip_apidoc_files` in the Makefile. | ||
| const skippedDocs = ['quic']; |
There was a problem hiding this comment.
https://github.com/nodejs/node/pull/66613/changes does a better job of keeping this in sync by reading the Makefile directly.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #66713 +/- ##
==========================================
+ Coverage 90.65% 90.66% +0.01%
==========================================
Files 797 797
Lines 278268 278268
Branches 53511 53513 +2
==========================================
+ Hits 252258 252302 +44
+ Misses 16384 16344 -40
+ Partials 9626 9622 -4 🚀 New features to boost your workflow:
|
We know that lightningcss resolves on release machines, so I assume Rolldown will as well, but I 100% want to double-check that on a release-ci after regular CI passes (a regular CI is not enough). |
There was a problem hiding this comment.
IMO this test is redundant with #66613 - only one should exist. if anything the logic could be combined
|
|
||
| global: { | ||
| input: [fromRoot('doc/api/*.md')], | ||
| // Keep in sync with `skip_apidoc_files` in the Makefile. |
There was a problem hiding this comment.
we load the makefile in the test - is it too much to do so here too. i cannot recall if that was discussed already in a configuration, or if we chose to make them static by convention
Co-authored-by: Brian Muenzenmeyer <brian.muenzenmeyer@gmail.com> Signed-off-by: avivkeller <me@aviv.sh>
This comment was marked as outdated.
This comment was marked as outdated.
Pass `skip_apidoc_files` from the Makefile to doc-kit with `--ignore` instead of duplicating the list in the config file, and read the list in test-make-doc.mjs synchronously (the callback `fs.open` was being awaited, which threw ERR_INVALID_ARG_TYPE). Assisted-by: Claude Fable 5.1
There was a problem hiding this comment.
Consider pulling out the test-docs.mjs changes to a separate PR that passes on the existing doc generation setup so we're more confident that this PR isn't changing behaviour?
ovflowd
left a comment
ovflowd
left a comment
There was a problem hiding this comment.
SgTm (Sorcerer Got Temu Merchandise)
This PR shouldn't change much from it's current form. Most changes will go in
doc-kit.test-make-docwas removed, since our new generator minifies the output HTML (so it's format is structurally inconsistent) and can't be easily parsed with the existing regular expressions.Furthermore, such tests are not needed due to our tests in doc-kit.
Preview: https://beta.docs.nodejs.org
Notable Change:
Node.js is thrilled to announce a brand new redesign of our documentation experience! The updated documentation now features HTML structure and styling that are much more closely aligned with the main Node.js website UI, creating a more consistent experience across the brand. You can check out the redesigned documentation today at
https://nodejs.org/api(Striked out since this isn't correct until this PR lands).