Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #644 +/- ##
==========================================
+ Coverage 75.90% 75.93% +0.02%
==========================================
Files 145 147 +2
Lines 13735 13910 +175
Branches 992 1001 +9
==========================================
+ Hits 10426 10562 +136
- Misses 3303 3342 +39
Partials 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
| File | Base | Head | Diff |
|---|---|---|---|
documentation.html |
36.92 KB | 50.38 KB | +13.46 KB (+36.44%) |
documentation.js |
9.74 KB | 21.57 KB | +11.83 KB (+121.41%) |
styles.css |
137.01 KB | 141.80 KB | +4.79 KB (+3.50%) |
index.js |
9.69 KB | 9.70 KB | +15.00 B (+0.15%) |
embedding.js |
37.29 KB | 37.30 KB | +5.00 B (+0.01%) |
intl.js |
37.08 KB | 37.08 KB | +5.00 B (+0.01%) |
synopsis.js |
15.72 KB | 15.72 KB | +5.00 B (+0.03%) |
There was a problem hiding this comment.
Pull request overview
Introduces a “Stability Overview” table to the new web generator output, mirroring the legacy generator’s ability to render a stability summary on the “About”/documentation page when slot tags are present.
Changes:
- Add
StabilityOverviewUI component (CSS + JSX) and register it inwebJSX import mapping. - Precompute and pass stability overview entries through the
jsx-astgenerator pipeline, injecting the table when the slot tag is present. - Ensure the
webgenerator creates the output directory before writing HTML/JS/CSS artifacts.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/generators/web/ui/components/StabilityOverview/index.module.css | Adds minimal layout styling for the stability overview table and badge cell. |
| src/generators/web/ui/components/StabilityOverview/index.jsx | Implements the stability overview table UI using Badge tooltips. |
| src/generators/web/generate.mjs | Creates config.output directory before emitting generated files. |
| src/generators/web/constants.mjs | Registers StabilityOverview in JSX_IMPORTS for bundling/import generation. |
| src/generators/jsx-ast/utils/buildContent.mjs | Threads stability overview entries through processEntry and injects the component when slot tags are present. |
| src/generators/jsx-ast/generate.mjs | Precomputes stability overview data once and passes it to workers via the extra stream context. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
avivkeller
left a comment
There was a problem hiding this comment.
I wonder if adding stability to the sidebar would be better?
It's should be on toc ? it's already |
I wonder if Aviv meant completely being there or maybe always being there? Although that seems a discussion for another issue. |
|
|
||
| const STABILITY_KINDS = ['error', 'warning', 'default', 'info']; | ||
|
|
||
| /** |
There was a problem hiding this comment.
if we remove it es-lint ask for it but it's doesn't require any jsdoc
There was a problem hiding this comment.
Just remove it or do proper JSdoc for it.
| .filter(node => node.stability?.children?.length) | ||
| .map(({ api, heading, stability }) => { | ||
| const [{ data }] = stability.children; | ||
| return { |
There was a problem hiding this comment.
Could this map fn be moved to a dedicated function and be unit tested? Thanks!
|
|
||
| // Pre-compute stability overview data once — avoid serialising full AST nodes to workers | ||
| const stabilityOverviewEntries = headNodes | ||
| .filter(node => node.stability?.children?.length) |
There was a problem hiding this comment.
| .filter(node => node.stability?.children?.length) | |
| .filter(node => !!node.stability?.children?.length) |
| ); | ||
|
|
||
| // Inject the stability overview table where the slot tag is present | ||
| if ( |
There was a problem hiding this comment.
I wonder how optimal this is... I wonder if a visit statement is better here, I also kinda dislike this method of inserting the stability index.
There was a problem hiding this comment.
This method would make sense for legacy docs. For the new docs, this method isn't really acceptable. The stability overview must be a React component now for the web generator, and all that we must do here is replace the STABILITY_OVERVIEW_SLOT_BEGIN or IDK with a slot for the component.
This would also make it easier to maintain the component.
i do that but aviv ask for AST approach #644 (comment) So what should I do ? |
|
I asked for an AST approach for generating a table, however, if that's not possible for a sidebar, we don't have to do it. I meant, the badge that you have in the table might be better suited in the sidebar, WDYT? |
I understand why an AST approach, but I'd prefer a React component here, we're using AST trees for building the structure of the page, which is fine I guess? But I'd rather have it being a React tree, not gonna lie. For example a lot of things are done in places that don't make much sense for me, for example https://github.com/nodejs/doc-kit/blob/main/src/generators/jsx-ast/utils/buildContent.mjs#L289-L316, we're building the document tree in JSX here by using this ast.mjs file (https://github.com/nodejs/doc-kit/blob/main/src/generators/jsx-ast/utils/ast.mjs) and this is awkward as hell for me and hard to maintain. If we ever want to make change to the document tree or customizing props, this is going to be incredibly hard for newcomers. I was never fan of this approach but initially it was simple, now it is overly complex. We're hand-crafting JSX elements here, where the reality is we could just make regular React Components (JSX Components/Elements).. The other part that hinders customization of doc-kit is that we already wire straight on the jsx-ast generator our own ui-components which limit in the future if people want to have other layouts/designs or use their own components for the structure of their docs. This is more of a long-term ask, but I do feel we should try to make the customization and maintainability here as easy as possible, but the jsx-ast seems to be doing too much stuff and too much hard-coded (https://github.com/nodejs/doc-kit/tree/main/src/generators/jsx-ast)... It'd be great if the Stability Overview is its own component. Could be on node-core/ui-components or specifically in the components folder on the web generator. |
|
Building the AST directly results in both faster, smaller, and more memory efficient builds. It's a little off putting for new comers, I suppose, however, thats nothing some documentation can't fix. I disagree that everything should be a client-side JSX element, I think that JSX elements should only be used when an AST implementation is not feasible. |
That's something that can definitely be changed. For instance, we allow customization of the logo, we can allow customization of everything else. |
|
@ovflowd if you have an issue with the current approach, feel free to open a designated issue and voice your concerns, which others can agree or refute |
You are literally creating JSX elements manually on jsx-ast... |
How you see customization of the tree or document or components with the current hard-coded jsx tree? |
|
Btw, I still believe this should be a React component. I'm 👎 with the current approach because I know we can make it simpler with React, but if that's too much trouble, I can waive my block. |
|
@avivkeller agree that I should comme back to react component ? |
Description
introduce stability overview on new web generator
Validation
It's should be cool 😎
Related Issues
Close #357
Check List
node --run testand all tests passed.node --run format&node --run lint.