Conversation
|
Review requested:
|
77ede22 to
3423c21
Compare
3423c21 to
451f8a7
Compare
cf2609b to
a3ce99d
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #57343 +/- ##
==========================================
- Coverage 89.80% 89.78% -0.03%
==========================================
Files 672 672
Lines 203907 203755 -152
Branches 39203 39167 -36
==========================================
- Hits 183121 182940 -181
- Misses 13113 13128 +15
- Partials 7673 7687 +14 🚀 New features to boost your workflow:
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
ovflowd
left a comment
There was a problem hiding this comment.
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!
This comment was marked as resolved.
This comment was marked as resolved.
It's not about the change being breaking, it's about giving Node.js collaborators the opportunity to weigh in on it – like we do for any other change. It also makes it easier for backporting ro reverting if need be.
Tell that to V8, clearly it does matter otherwise you wouldn't have needed to add that |
Edit: What I meant here is that, there's no effort of actual backporting needed here as these changes are supposed to be backwards compatible. So these changes are safe to be backported without requiring outside/external or internal patches.
It matters because you structured you test to grab the keys in said order. That is an array, array are ordered. JSON objects aren't ordered. JavaScript objects "are", but we're talking about JSON here, not JavaScript. |
|
The
notable-change
Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section. |
|
(I've added
notable-change
|
|
I've started working through the bits and pieces here and the conversation, working on trying to identify a compromise position that can allow this to proceed forward. Here's where I'm landing currently but it might evolve a bit as I dig in further:
I get there are nuances in this that I probably don't yet fully have my head around so I'm continuing to catch up. Will comment further once I feel confident that I'm adequately up to speed. |
|
@aduh95 if we land this as semver-major, will all your concerns be resolved? If so, could you label + approve the PR? |
|
I'm really hoping we don't end up landing this as semver-major – also there seems to be a consensus that this PR is very unlikely to be breaking. My preferred outcome would be that we land nodejs/doc-kit#595, include it in this PR, and the additional keys can be introduced later in follow up PRs. |
|
Sorry for the digression.
Unfortunately this is imprecise. The order matters whenever it's processed by JavaScript. Essentially: It's a valid JSON document. When parsed in JS (and possibly everywhere), it would result into: Changing the order of keys mostly does not matter. I don't think it matters in this case anyway, but please check for duplicates. I don't think adding new properties is a breaking change either (removing would be). I would prefer them not be there unless strictly needed (as to reduce the surface area). |
|
That's a good point. Although as you said correctly, doesn't apply here 😅 Edit: To be clear, I stand corrected by Matteo, he's technically correct and corrected my previous statement. I just don't think that in this case such issue could happen here. |
|
No it does apply, in the test file we have And FWIW I definitely agree that the current JSON output is in a less than ideal, and a reform would be welcome – but in a follow up semver-major PR. |
|
I wanted to clarify a few points regarding the JSON structure and why the ordering has changed. First, it's important to note that the top-level keys (which dictate the content order from the source Markdown) remain intact. We aren't introducing any breaking changes to the overall sequence of the document content. Regarding the inner keys of each entry:
Since JSON objects are inherently unordered, this change shouldn't break any standard parsing. Unless a downstream consumer is accessing keys by index (e.g., While we can refactor the new tooling to mirror the legacy order if it’s considered a blocker, it feels like an unnecessary use of resources for a change that doesn't provide functional value. The current test failures seem to be a result of the tests expecting a specific string-matching order rather than validating the schema itself. @aduh95, could you provide a real-world example of where this specific internal key order would break downstream usage? It would help to understand if there's a dependency I'm overlooking. To clarify for everyone else, the concern is that keys like
Note: The highlighted keys are just examples of the shift in placement. Even in the legacy tooling, this order wasn't always 100% consistent across different files. |
Me trying to review this PR. With keys in a different order, it's very hard to review. With nodejs/doc-kit#595, at least I'm able to diff which allows me to see that e.g.
The HTML version is also suffering from the lack of parsing:
I haven't investigated, that's likely due to the conflict on The sub-keys being in a different order does produce a lot of diff in the output, it's annoying but I guess it's workable. BTW there are indeed lots of bug fixes, so good work! |
That's an issue with the markdown. See #61756 |
Gotcha, for manual comparison of the JSON this makes sense. I guess we can land nodejs/doc-kit#595
Do you mean to say the "node:process's 'message' event params are no longer parsed correctly:" is an issue on the markdown or? |
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> Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
3b3a9e2 to
5f45cfa
Compare




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
Notable Change info (by @avivkeller):
The Node.js Website and Website Infrastructure teams have introduced a brand-new documentation pipeline, modernizing how our API docs are generated. While the documentation site may look familiar today, this infrastructure we are hard at work making a completely refreshed user interface in the very near future!