nodejs / api-docs-tooling

Node.js's tooling for API generation
https://nodejs.org/api
MIT License
5 stars 5 forks source link

feat: legacy json generator #142

Open RedYetiDev opened 1 week ago

RedYetiDev commented 1 week ago

Closes #57 Closes #141 Closes #92


This is an extension of #92. This is a seperate PR due to a rebase that caused merge conflicts.

Description

See https://github.com/nodejs/api-docs-tooling/pull/92 for a description

Validation

# Variables
FILENAME="$1"
NODE="path/to/node/repo"
API_DOCS_TOOLING="path/to/api-docs/tooling/repo"
DOC="$NODE/doc/api/$FILENAME.md"
OUTPUT_DIR="output"
NEW_DIR="$OUTPUT_DIR/new"
OLD_DIR="$OUTPUT_DIR/old"
SORT_JSON=$(cat <<EOF
def sorted_walk(f):
  . as \$in
  | if type == "object" then
      reduce keys[] as \$key
        ( {}; . + { (\$key):  (\$in[\$key] | sorted_walk(f)) } ) | f
  elif type == "array" then map( sorted_walk(f) ) | f
  else f
  end;

def normalize: sorted_walk(if type == "array" then sort else . end);

normalize
EOF
)

# Functions
initialize_output() {
  rm -rf "$OUTPUT_DIR" && mkdir -p "$NEW_DIR" "$OLD_DIR"
}

generate_new() {
  node "$API_DOCS_TOOLING/bin/cli.mjs" -i "$DOC" -t legacy-json -o "$NEW_DIR"
}

generate_old() {
  node "$NODE/tools/doc/generate.mjs" "$DOC" --output-directory="$OLD_DIR"
}

compare_outputs() {
  git diff --no-index <(jq --sort-keys -f <(echo $SORT_JSON) "$OLD_DIR/$FILENAME.json") <(jq --sort-keys -f <(echo $SORT_JSON) "$NEW_DIR/$FILENAME.json")
}

# Main logic
initialize_output
generate_new
generate_old
compare_outputs

Validate with bash ./file.sh addons, bash ./file.sh fs, etc.

Differences

These are the small differences, and likely do not need to be fixed.

  1. Highlighting is done by Shiki instead of Highlight.js
  2. The old parser had issues parsing optional parameters in some cases (IIUC)
  3. The old parser mistook return-ed Objects for additional parameters
  4. The old parser pre-parsed the source_code YAML comments, ours does not.
  5. The old parser would use the filename as the displayName for the root node, this one uses the actual display name. (E.X. fs (old) vs File System (new))
  6. Links are not preserved in textRaw (I.E. [`something`][] in the old parser is `something` in the new one)
  7. meta is always a property.

Check List

RedYetiDev commented 1 week ago

@flakey5 PTAL

RedYetiDev commented 1 week ago

Currently, this PR is 1:1 with the original JSON (AFAIK), except for the following differences, which probably don't need to be fixed:

  1. Highlighting is done by Shiki instead of Highlight.js
  2. The old parser had issues parsing optional parameters in some cases (IIUC)
  3. The old parser mistook return-ed Objects for additional parameters
  4. The old parser pre-parsed the source_code YAML comments, ours does not.
  5. The old parser would use the filename as the displayName for the root node, this one uses the actual display name. (E.X. fs (old) vs File System (new))
ovflowd commented 1 week ago

So, is it correct to assume that this is PR supersedes @flakey5's PR? Was this communicated with @flakey5? Are they fine with the continuation on this PR?

Note that you could also have pushed your work on their PR if they were fine :) -- or at least you can once you join the team on GitHub I believe.

I will review this once I get time 🙏

RedYetiDev commented 1 week ago

I would wait until we get the go-ahead from the other PR. My plan was to open this PR into that branch, but there was merge conflicts. If we don't get the go ahead, I suggest that @flakey5 use git apply to semi-manually apply some of these changes.

(I obviously don't want to take away from the other PR / cut off @flakey5's amazing work, and if we'd rather keep the discussion there, I am fine with that)

ovflowd commented 1 week ago

BTW @RedYetiDev CodeQL complained about your code, that it might have a security vuln; Mind giving an eye? 👀

RedYetiDev commented 1 week ago

BTW @RedYetiDev CodeQL complained about your code, that it might have a security vuln; Mind giving an eye? 👀

https://github.com/nodejs/api-docs-tooling/pull/142#discussion_r1835752936

flakey5 commented 1 week ago

I was either traveling or getting over jet lag this weekend so I wasn't able to respond much to the comments made on #92, I would've appreciated a little more time to respond to the comments made but it's water under the bridge.

Imo it makes sense to continue with this one since the commits are already here and it's pretty much done, still need to make sure the unresolved comments in #92 are addressed here however

RedYetiDev commented 1 week ago

I would've appreciated a little more time to respond to the comments made but it's water under the bridge.

Sorry!! Thank you for understanding. Again, I'm happy to instead have this merged into your PR, I really don't want to take over your amazing work

RedYetiDev commented 5 days ago

src/generators/legacy-json/utils/buildSection.mjs: Evaluated as low risk

:laughing: It didn't even review the most important file

RedYetiDev commented 4 days ago

Thanks for the review! I'll check it out and update the code later today!

RedYetiDev commented 1 day ago

I’ve attempted to resolve the review comments, when you get a chance, let me know if there are any other concerns.

ovflowd commented 1 day ago

(BTW, I wanted to clarify that this is fantastic work! I enormously appreciate your effort here!)

RedYetiDev commented 1 day ago

(BTW, I wanted to clarify that this is fantastic work! I enormously appreciate your effort here!)

And I appreciate your reviews! And @flakey5's initial PR!

RedYetiDev commented 1 day ago

I think we're almost there. Left a final round of comments. I'd also like to have some benchmarks, how long does the tooling take to run with all the doc files for only the legacy-json generator, and if long enough would you be able to attach a debugger and find slow paths?

I'll look for slow paths. Right now (for fs):

real    0m1.765s
user    0m2.718s
sys     0m0.260s
RedYetiDev commented 1 day ago

there really aren't too many slow paths, I think we are good: image

The really long "on ignore list" at the beginning is Remark.