-
Notifications
You must be signed in to change notification settings - Fork 43
feat(web): introduce stability overview
#644
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
113fbbe
df3c7a5
95229d1
f2b9366
83203ee
cc4cdf7
e0d70a9
5f3295a
770ae29
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| import assert from 'node:assert/strict'; | ||
| import { describe, it } from 'node:test'; | ||
|
|
||
| import buildStabilityOverview from '../buildStabilityOverview.mjs'; | ||
|
|
||
| const getAttribute = (node, name) => | ||
| node.attributes.find(attribute => attribute.name === name)?.value; | ||
|
|
||
| const getAttributeExpression = (node, name) => | ||
| getAttribute(node, name)?.data?.estree?.body?.[0]?.expression; | ||
|
|
||
| describe('buildStabilityOverview', () => { | ||
| it('builds a StabilityOverview JSX block element', () => { | ||
| const entries = [ | ||
| { | ||
| api: 'fs', | ||
| name: 'File system', | ||
| stabilityIndex: 2, | ||
| stabilityDescription: 'Stable', | ||
| }, | ||
| { | ||
| api: 'async_context', | ||
| name: 'Async context', | ||
| stabilityIndex: 1, | ||
| stabilityDescription: 'Experimental', | ||
| }, | ||
| ]; | ||
|
|
||
| const result = buildStabilityOverview(entries); | ||
|
|
||
| assert.equal(result.type, 'mdxJsxFlowElement'); | ||
| assert.equal(result.name, 'StabilityOverview'); | ||
| }); | ||
|
|
||
| it('serializes entries into the entries prop', () => { | ||
| const result = buildStabilityOverview([ | ||
| { | ||
| api: 'fs', | ||
| name: 'File system', | ||
| stabilityIndex: 0, | ||
| stabilityDescription: 'Deprecated: use fs/promises', | ||
| }, | ||
| { | ||
| api: 'timers', | ||
| name: 'Timers', | ||
| stabilityIndex: 2, | ||
| stabilityDescription: 'Stable', | ||
| }, | ||
| ]); | ||
|
|
||
| const entriesExpression = getAttributeExpression(result, 'entries'); | ||
|
|
||
| assert.equal(entriesExpression.type, 'ArrayExpression'); | ||
| assert.equal(entriesExpression.elements.length, 2); | ||
|
|
||
| const firstEntry = entriesExpression.elements[0]; | ||
| const firstApi = firstEntry.properties.find( | ||
| ({ key }) => key.name === 'api' | ||
| ); | ||
| const firstStabilityIndex = firstEntry.properties.find( | ||
| ({ key }) => key.name === 'stabilityIndex' | ||
| ); | ||
|
|
||
| assert.equal(firstApi.value.value, 'fs'); | ||
| assert.equal(firstStabilityIndex.value.value, 0); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import assert from 'node:assert/strict'; | ||
| import { describe, it } from 'node:test'; | ||
|
|
||
| import { processEntry } from '../buildContent.mjs'; | ||
|
|
||
| describe('processEntry', () => { | ||
| it('does not throw when tags are missing', () => { | ||
| const entry = { | ||
| content: { | ||
| type: 'root', | ||
| children: [], | ||
| }, | ||
| }; | ||
|
|
||
| assert.doesNotThrow(() => | ||
| processEntry(entry, null, [ | ||
| { | ||
| api: 'fs', | ||
| name: 'File system', | ||
| stabilityIndex: 2, | ||
| stabilityDescription: 'Stable', | ||
| }, | ||
| ]) | ||
| ); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ import { SKIP, visit } from 'unist-util-visit'; | |
|
|
||
| import { createJSXElement } from './ast.mjs'; | ||
| import { buildMetaBarProps } from './buildBarProps.mjs'; | ||
| import buildStabilityOverview from './buildStabilityOverview.mjs'; | ||
| import { enforceArray } from '../../../utils/array.mjs'; | ||
| import { JSX_IMPORTS } from '../../web/constants.mjs'; | ||
| import { | ||
|
|
@@ -256,7 +257,7 @@ export const transformHeadingNode = async ( | |
| * @param {import('../../metadata/types').MetadataEntry} entry - The API metadata entry to process | ||
| * @param {import('unified').Processor} remark - The remark processor | ||
| */ | ||
| export const processEntry = (entry, remark) => { | ||
| export const processEntry = (entry, remark, stabilityOverviewEntries = []) => { | ||
| // Deep copy content to avoid mutations on original | ||
| const content = structuredClone(entry.content); | ||
|
|
||
|
|
@@ -276,6 +277,14 @@ export const processEntry = (entry, remark) => { | |
| (parent.children[idx] = createSignatureTable(node, remark)) | ||
| ); | ||
|
|
||
| // Inject the stability overview table where the slot tag is present | ||
| if ( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc @avivkeller I really do wonder if there's an alternative here, because you're kinda always passing the stability overview entries in the hope of finding this, I'd argue it should be done differently/somewhere else?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bump, @avivkeller
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Brainstorming:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm big +1 on aviv idea for index.html with that we will be able to generate a grid layout of pages with title + description + stability if present |
||
| stabilityOverviewEntries.length && | ||
| entry.tags?.includes('STABILITY_OVERVIEW_SLOT_BEGIN') | ||
| ) { | ||
| content.children.push(buildStabilityOverview(stabilityOverviewEntries)); | ||
| } | ||
|
|
||
| return content; | ||
| }; | ||
|
|
||
|
|
@@ -290,13 +299,16 @@ export const createDocumentLayout = ( | |
| entries, | ||
| sideBarProps, | ||
| metaBarProps, | ||
| remark | ||
| remark, | ||
| stabilityOverviewEntries = [] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @avivkeller I'm not a fan that this fn is becoming bigger and bigger with sideBarProps, metaBarProps, stabilityOverviewEntries, could we create a share Map for storing these things so we don't need to pass down within fns? This can be done in a follow-up PR, but I want to keep these methods clean.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, I'm sure we can simplify it somehow.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @avivkeller can you open an issue? |
||
| ) => | ||
| createTree('root', [ | ||
| createJSXElement(JSX_IMPORTS.Layout.name, { | ||
| sideBarProps, | ||
| metaBarProps, | ||
| children: entries.map(entry => processEntry(entry, remark)), | ||
| children: entries.map(entry => | ||
| processEntry(entry, remark, stabilityOverviewEntries) | ||
| ), | ||
| }), | ||
| ]); | ||
|
|
||
|
|
@@ -310,7 +322,13 @@ export const createDocumentLayout = ( | |
| * @param {import('unified').Processor} remark - Remark processor instance for markdown processing | ||
| * @returns {Promise<JSXContent>} | ||
| */ | ||
| const buildContent = async (metadataEntries, head, sideBarProps, remark) => { | ||
| const buildContent = async ( | ||
| metadataEntries, | ||
| head, | ||
| sideBarProps, | ||
| remark, | ||
| stabilityOverviewEntries = [] | ||
| ) => { | ||
| // Build props for the MetaBar from head and entries | ||
| const metaBarProps = buildMetaBarProps(head, metadataEntries); | ||
|
|
||
|
|
@@ -319,7 +337,8 @@ const buildContent = async (metadataEntries, head, sideBarProps, remark) => { | |
| metadataEntries, | ||
| sideBarProps, | ||
| metaBarProps, | ||
| remark | ||
| remark, | ||
| stabilityOverviewEntries | ||
| ); | ||
|
|
||
| // Run remark processor to transform AST (parse markdown, plugins, etc.) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| import { createJSXElement } from './ast.mjs'; | ||
| // TODO(@avivkeller): JSX imports belong in the JSX generator | ||
| import { JSX_IMPORTS } from '../../web/constants.mjs'; | ||
AugustinMauroy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /** | ||
| * Builds the Stability Overview component. | ||
| * | ||
| * @param {Array<{ api: string, name: string, stabilityIndex: number, stabilityDescription: string }>} entries | ||
| * @returns {import('unist').Node} | ||
| */ | ||
| const buildStabilityOverview = entries => | ||
| createJSXElement(JSX_IMPORTS.StabilityOverview.name, { | ||
| inline: false, | ||
| entries, | ||
| }); | ||
|
|
||
| export default buildStabilityOverview; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export const STABILITY_KINDS = ['error', 'warning', 'default', 'info']; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| import BadgeGroup from '@node-core/ui-components/Common/BadgeGroup'; | ||
|
|
||
| import { STABILITY_KINDS } from './constants.mjs'; | ||
|
|
||
| /** | ||
| * Renders the module stability overview table. | ||
| * @param {{ entries: Array<{ api: string, name: string, stabilityIndex: number, stabilityDescription: string }> }} props | ||
| */ | ||
| export default ({ entries = [] }) => ( | ||
| <table> | ||
| <thead> | ||
| <tr> | ||
| <th>API</th> | ||
| <th>Stability</th> | ||
| </tr> | ||
| </thead> | ||
| <tbody> | ||
| {entries.map(({ api, name, stabilityIndex, stabilityDescription }) => ( | ||
| <tr key={api}> | ||
| <td> | ||
| <a href={`${api}.html`}>{name}</a> | ||
| </td> | ||
| <td> | ||
| <BadgeGroup | ||
| as="span" | ||
| size="small" | ||
| kind={STABILITY_KINDS[stabilityIndex] ?? 'success'} | ||
| badgeText={stabilityIndex} | ||
| > | ||
| {stabilityDescription} | ||
| </BadgeGroup> | ||
| </td> | ||
| </tr> | ||
| ))} | ||
| </tbody> | ||
| </table> | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this map fn be moved to a dedicated function and be unit tested? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seem reasonable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still waiting for that 👀