feat(jsx): add jsx generator
#273
Conversation
|
I'm working on skipping the MDX and going to straight to JSX, so I'll add lots of tests when that's started |
avivkeller
force-pushed
the
wip/mdx-generator
branch
2 times, most recently
from
60922b4 to
d2bd5f4
Compare
|
This uses pnpm for now, see #236 for reference. If we do adopt pnpm, it would be a seperate PR, this is just for development. |
avivkeller
force-pushed
the
wip/mdx-generator
branch
from
8020c31 to
60e76e5
Compare
| @@ -15,7 +15,8 @@ | |||
| "test:watch": "node --test --watch", | |||
| "prepare": "husky", | |||
| "run": "node bin/cli.mjs", | |||
| "watch": "node --watch bin/cli.mjs" | |||
| "watch": "node --watch bin/cli.mjs", | |||
| "postinstall": "git-deps install" | |||
There was a problem hiding this comment.
If this is the intended behavior and the solution, we should move this dependency to node.js organization, before merging this pull-request. It will run on every postinstall, and it's huge security concern.
There was a problem hiding this comment.
We are still looking for solutions, see #236
There was a problem hiding this comment.
Agreed with @anonrig although I'd say this is a draft, but we def... need to find a perma solution.
There was a problem hiding this comment.
We should at the very least move this dep to @pkgjs. @avivkeller feel free to open an issue on /admin to transfer this + ownership.
There was a problem hiding this comment.
Shouldn't we first decide whether this is a perma solution? I'd argue that we shouldn't maintain a package that we aren't going to use-but I'm happy to transfer it to the org regardless
There was a problem hiding this comment.
Sure, but this PR cannot land till we decide that, or at least cannot land whilst git-deps is a dependency here 😅
There was a problem hiding this comment.
Sure, but this PR cannot land till we decide that
Per the vote in #236, it looks like nodejs/nodejs.org#7776 will resolve that, and once they are published, I'll switch it. git-deps allows me to test this PR until then.
avivkeller
force-pushed
the
wip/mdx-generator
branch
from
ef09a04 to
a002cc3
Compare
| * | ||
| * @param {string} name - The name of the JSX element | ||
| * @param {{ | ||
| * inline?: boolean, |
There was a problem hiding this comment.
Lines 11 to 13 should be indented
There was a problem hiding this comment.
There was a problem hiding this comment.
Right, but I believe extra level, makes it easier to read:
/**
* Creates an MDX JSX element with support for complex attribute values.
*
* @param {string} name - The name of the JSX element
* @param {{
* inline?: boolean,
* children?: string | import('unist').Node[],
* [key: string]: any
* }} [options={}] - Options including type, children, and JSX attributes
* @returns {import('unist').Node} The created MDX JSX element node
*/
There was a problem hiding this comment.
- The lint rule is enforced
- We don't indent anywhere else, i.e. in
createMetadataorlegacy-html
This can be changed, but it breaks our conventions
| * @param {ApiDocMetadataEntry} head - Main API metadata entry | ||
| * @param {Array<ApiDocMetadataEntry>} entries - All API metadata entries | ||
| */ | ||
| export function buildMetaBarProps(head, entries) { |
There was a problem hiding this comment.
Maybe explain a bit more why we need to append all this text
There was a problem hiding this comment.
Like what, does // Extract text content for reading time calculation not sum it up? readingTime expects text, not AST.
| addedIn: head.introduced_in || head.added_in || '', | ||
| readingTime: readingTime(textContent).text, | ||
| viewAs: [['JSON', `${head.api}.json`]], | ||
| editThisPage: `${DOC_API_BLOB_EDIT_BASE_URL}${head.api}.md`, |
There was a problem hiding this comment.
Don't we have a metadata property that is exactly this already?
There was a problem hiding this comment.
I don't think so, we use the same method of building it in legacy-html.
There was a problem hiding this comment.
Might make sense to then make this as a property on the metadata itself
There was a problem hiding this comment.
I don't think it's needed. It's really just {constant}{metadata.api}.md, so it can very easily be built, however, if you insist, maybe that can be a follow up.
| */ | ||
| const createContentStructure = (entries, sideBarProps, metaBarProps) => { | ||
| return createTree('root', [ | ||
| createJSXElement('NavBar'), |
There was a problem hiding this comment.
Since all these are React Components, would make sense to reference these as constants such as:
import { NavBar } from '@node-core/ui....'
....
createJSXElement(NavBar.displayName), // or something similar
This ensures that we use their real names; And then wherever we expose them (be it next.mdx.mjs and next.client.mdx.mjs) we also use the same constants.
This ensures that this won't randomly break if we chan the declared Components within MDX or somewhere else. Apply this to all components you're invoking within this new generator.
There was a problem hiding this comment.
Most of these components haven't been implemented yet, can this be done when they are implemented?
| // TODO(@avivkeller): Our parsers shouldn't return raw nodes | ||
| // When they mistake "<Type>" for an HTML node, rather than | ||
| // the string type that it is. | ||
| if (node.type === 'raw') { |
There was a problem hiding this comment.
I'd rather have a switch statement here :P
There was a problem hiding this comment.
Or simply have text within TYPE_TRANSFORMS
There was a problem hiding this comment.
I'd rather have a switch statement here :P
There's only one clause, so I think we should leave it as is.
Or simply have
textwithinTYPE_TRANSFORMS
The transforms we are apply are for .tagName, not .type
This PR introduces a
jsxgenerator that transforms Markdown AST and metadata entries into a JSX (recma) AST (with a root ofProgram), which can then be compiled by thereact-webgenerator from #7.The JSX generator produces AST that assumes the following components have been implemented:
CodeBox: Implementation matches nodejs.org CodeBoxCodeTabs: Implementation matches nodejs.org CodeTabsBlockquote: Import from@node-core/ui-components/Common/BlockquoteAlertBox: Import from@node-core/ui-components/Common/AlertBoxCircularIcon: Accepts:symbol: Icon emblem (e.g., "C" or "E")color: Icon color (e.g., "red" or "#FFFFF")Design
ChangeHistory: Similar to@node-core/ui-components/Common/Select, accepts:changes: Array of{ versions: string[], label: string, url?: string }(whereversions[]contains semver strings likev1.2.3)Design
NavBar: Wrapper of@node-core/ui-components/Containers/NavBar, accepts no propsFooter: Wrapper of@node-core/ui-components/Containers/Footer, accepts no propsArticle: Import from@node-core/ui-components/Containers/ArticleSideBar: Similar to@node-core/ui-components/Container/SideBar, accepts:versions: Array of versions (e.g.,v20.0.0) where the document existscurrentVersion: Current documentation version (e.g.,v20.1.0)currentPage: Current page (e.g.,async_context.html) for highlighting in the sidebardocPages: Array of[{ title: string, headings: [title, hash][], doc: string }]pairs (e.g.,{ title: "Globals", headers: [["someGlobal", "#some-global"]], doc: "globals.html"})Design
MetaBar: Wrapper of@node-core/ui-components/Containers/MetaBar, accepts:headings: Array of{ depth: number, value: string }for table of contentsaddedIn: Version when doc/module was added (e.g.,v0.10.0)readingTime: String like"9 min read"viewAs: Array of["Type", "URL"]pairs (currently only supports["JSON", "globals.json"])editThisPage: GitHub URL for editing the pageAs a reminder, the Figma design is:
