feat: legacy json generator #92
Conversation
|
Marking ready for review since there shouldn't be any big changes left, but note this isn't generating a full 1:1 match with the original generator yet. There's still some fixes and testing I need to do |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Can you describe some of the notorious changes? Some discrepancies might be fine :) |
|
|
||
| const previousEntry = entries[i - 1]; | ||
|
|
||
| const previousDepth = previousEntry.heading.depth; |
There was a problem hiding this comment.
Interesting way of iterating this. Wouldn't it be easier to use the heading section topology we already created, which has heading info that tells which depth you are based on the heading depth?
There was a problem hiding this comment.
Wouldn't it be easier to use the heading section topology we already created
Wdym?
There was a problem hiding this comment.
I was thinking you can use unified here, to iterate through the headings, (which are nodes) and then automatically add children to each parent.
I think I could explain this over a call, but pretty much using the visit API to self reference the previous heading in terms of depth, and then visit next levels.
There was a problem hiding this comment.
My fear is that the code below is too much convoluted and hacky 🙈
Closes #57 Signed-off-by: flakey5 <[email protected]>
Signed-off-by: flakey5 <[email protected]>
Co-authored-by: Augustin Mauroy <[email protected]>
Signed-off-by: flakey5 <[email protected]>
Signed-off-by: flakey5 <[email protected]>
Signed-off-by: flakey5 <[email protected]>
Signed-off-by: flakey5 <[email protected]>
Signed-off-by: flakey5 <[email protected]>
Signed-off-by: flakey5 <[email protected]>
Signed-off-by: flakey5 <[email protected]>
Co-authored-by: Caner Akdas <[email protected]>
Signed-off-by: flakey5 <[email protected]>
Signed-off-by: flakey5 <[email protected]>
Co-authored-by: Augustin Mauroy <[email protected]>
Signed-off-by: flakey5 <[email protected]>
|
@flakey5 what is still missing here? |
| return section; | ||
| }; | ||
|
|
||
| await Promise.all( |
There was a problem hiding this comment.
Can't we use a for here? To avoid using Promise.all?
There was a problem hiding this comment.
This is what was originally done, then after an initial review, we put a Promise.all instead of a series of awaits in a loop.
There was a problem hiding this comment.
Am I stuttering O.o -- I still believe await would be better here for readability
There was a problem hiding this comment.
in terms of code readability, it's the same for me. But from what I've been able to understand in this kind of case it's better for performance to use Promise.all.
There was a problem hiding this comment.
I've been able to understand in this kind of case it's better for performance to use Promise.all.
Why?
There was a problem hiding this comment.
Why?
Unfortunately I don't remember much about it, but I'd seen a blog post about it.
There was a problem hiding this comment.
Ah yes, but that would only be the case if there's threading. It is correct to say that with the Promise approach at least the Promises will all be dispatched in parallel, but they will be executed one per time.
So in the end the feeling might be that it is faster, although it might not be. I do believe await syntax would better suit here, but no strong feelings.
| return { | ||
| textRaw: text, | ||
| type: head.data.type, | ||
| name: text.toLowerCase().replaceAll(' ', '_'), |
| parameter = undefined; | ||
|
|
||
| // Try finding a parameter this is being shared with | ||
| for (const otherParam of rawParameters) { |
There was a problem hiding this comment.
This seems a bit convoluted. Do you believe there's something you can do here to optimise the code?
I can see a few ways of optimising the signature generation and reducing its footprint.
| function textJoin(nodes) { | ||
| return nodes | ||
| .map(node => { | ||
| switch (node.type) { |
There was a problem hiding this comment.
Can the body of this map be extracted to a function?
| /** | ||
| * @param {Array<import('mdast').PhrasingContent>} nodes | ||
| */ | ||
| function textJoin(nodes) { |
There was a problem hiding this comment.
Are you just making this Markdown-ish? We already have this https://github.com/nodejs/api-docs-tooling/blob/main/src/utils/unist.mjs#L12
| } | ||
|
|
||
| // Render the description as if it was html | ||
| section.desc = unified() |
There was a problem hiding this comment.
No need to do this, can you simply call this? https://github.com/nodejs/api-docs-tooling/blob/main/src/utils/remark.mjs#L20
We don't need remark-html, remark-rehype is good enough and does the same :)
(remark-rehype then rehype-stringify)
There was a problem hiding this comment.
You can also remove the depenendecy of remark-html
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Signed-off-by: flakey5 <[email protected]>
| * @param {Array<import('../types.d.ts').List} markdownParameters The properties in the AST | ||
| * @returns {import('../types.d.ts').MethodSignature | undefined} | ||
| */ | ||
| export default (textRaw, markdownParameters) => { |
There was a problem hiding this comment.
todo this is broken still
| function createMeta(entry) { | ||
| const makeArrayIfNotAlready = val => (Array.isArray(val) ? val : [val]); | ||
|
|
||
| const { added_in, n_api_version, deprecated_in, removed_in, changes } = entry; | ||
| return { | ||
| changes, | ||
| added: added_in ? makeArrayIfNotAlready(added_in) : undefined, | ||
| napiVersion: n_api_version | ||
| ? makeArrayIfNotAlready(n_api_version) | ||
| : undefined, | ||
| deprecated: deprecated_in | ||
| ? makeArrayIfNotAlready(deprecated_in) | ||
| : undefined, | ||
| removed: removed_in ? makeArrayIfNotAlready(removed_in) : undefined, | ||
| }; | ||
| } |
There was a problem hiding this comment.
| function createMeta(entry) { | |
| const makeArrayIfNotAlready = val => (Array.isArray(val) ? val : [val]); | |
| const { added_in, n_api_version, deprecated_in, removed_in, changes } = entry; | |
| return { | |
| changes, | |
| added: added_in ? makeArrayIfNotAlready(added_in) : undefined, | |
| napiVersion: n_api_version | |
| ? makeArrayIfNotAlready(n_api_version) | |
| : undefined, | |
| deprecated: deprecated_in | |
| ? makeArrayIfNotAlready(deprecated_in) | |
| : undefined, | |
| removed: removed_in ? makeArrayIfNotAlready(removed_in) : undefined, | |
| }; | |
| } | |
| function createMeta(entry) { | |
| const makeArrayIfNotAlready = val => (Array.isArray(val) ? val : [val]); | |
| const { added_in, n_api_version, deprecated_in, removed_in, changes } = entry; | |
| if (!added_in && !n_api_version && !deprecated_in && !removed_in && changes.length < 1) { | |
| return undefined; | |
| } | |
| return { | |
| changes: changes.length > 0 ? changes : undefined, | |
| added: added_in ? makeArrayIfNotAlready(added_in) : undefined, | |
| napiVersion: n_api_version | |
| ? makeArrayIfNotAlready(n_api_version) | |
| : undefined, | |
| deprecated: deprecated_in | |
| ? makeArrayIfNotAlready(deprecated_in) | |
| : undefined, | |
| removed: removed_in ? makeArrayIfNotAlready(removed_in) : undefined, | |
| }; | |
| } |
In the old generator, if there was no metadata, the property was emitted entirely.
|
Hey @flakey5. I've made some local modifications to this, and it now generates (kind of?) 1:1 code (still need to work out some kinks). Some of my changes are suggestions in the review above, but if you'd like, I can open a PR into your branch? |
|
Good news! My local changes are almost 1:1 with the original JSON. The differences are:
Changes: 44d12f5 |
Co-authored-by: Aviv Keller <[email protected]>
That sgtm! Thank you! |
|
I tried to open a PR, but it has a conflict, probably because I rebased. You can still apply the changes with |
|
I now believe my modifications are as close to 1:1 as possible. I'll do some final checks, but if that's the case, I'll open a second PR stemming off of this one, as I can't merge them due to conflicts. |
|
I hope you don't mind I opened #142. If you mind, you can use the data from that PR and merge it into this one, but that'll have conflicts, and I don't have the access to do it myself. |
|
Superseded by #142 |
Description
Legacy json generator aiming to repro 1:1 the existing json format for docs
Still a sizeable amount left to be done, so opening this as a draft.
TODO
Returns: {boolean}instead ofReturns: [](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#Boolean_type))introduced_indoesn't work?Invalid param "{ lineLengths }"errorValidation
mkdir original && mkdir new node node/tools/doc/generate.mjs node/doc/api/addons.md --output-directory original/ node api-docs-tooling/bin/cli.mjs -i node/doc/api/addons.md -t legacy-json -o new/ git diff original new