Skip to content
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

feat apilinks.json generator #153

Merged
merged 2 commits into from
Feb 11, 2025
Merged

feat apilinks.json generator #153

merged 2 commits into from
Feb 11, 2025

Conversation

flakey5
Copy link
Member

@flakey5 flakey5 commented Nov 29, 2024

Closes #152

Opening this as a draft currently to get feedback on the approach since it's a bit non-trivial relative to the other generators. The apilinks.json file maps things exported by modules to their source locations on Github.

Example:

{
  "SomeClass.prototype.var": "github.com/nodejs/node/tree/<hash>/lib/file.js#L100"
}

This means we need to parse the module's javascript source in addition to its markdown.

What the current approach does is doing:

  • Adds another loading & parsing step for the js source files
    • acorn is used for parsing the source files
    • This intakes .js files, so to run it you need to do pass in node/lib/*.js as the input
    • This is dependent on the Markdown source parsing since it uses the source_link metadata in the docs
  • Exposes the parsed js ast to other generators by adding the ast-js generator
  • api-links generator is based off of the ast-js result

@flakey5 flakey5 requested a review from a team as a code owner November 29, 2024 07:59
@flakey5 flakey5 marked this pull request as draft November 29, 2024 07:59
src/utils/git.mjs Fixed Show fixed Hide fixed
src/utils/git.mjs Fixed Show fixed Hide fixed
src/utils/git.mjs Fixed Show fixed Hide fixed
src/utils/git.mjs Fixed Show fixed Hide fixed
src/utils/git.mjs Outdated Show resolved Hide resolved
src/parser.mjs Outdated Show resolved Hide resolved
@ovflowd
Copy link
Member

ovflowd commented Dec 19, 2024

@flakey5 do you need review here? I think I forgot to review this PR!

@flakey5
Copy link
Member Author

flakey5 commented Dec 20, 2024

do you need review here

As far as the approach yes please

bin/cli.mjs Outdated Show resolved Hide resolved
bin/cli.mjs Outdated Show resolved Hide resolved
bin/cli.mjs Outdated Show resolved Hide resolved
src/generators.mjs Outdated Show resolved Hide resolved
src/generators.mjs Outdated Show resolved Hide resolved
src/loader.mjs Outdated Show resolved Hide resolved
src/loader.mjs Outdated Show resolved Hide resolved
src/parser.mjs Outdated Show resolved Hide resolved
src/utils/git.mjs Outdated Show resolved Hide resolved
Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach in general sounds good; Just, some of these utilities are way too complex and big and the git manipulation scripts are possibly dangerous or at least an easy vulnerability hell.

@ovflowd
Copy link
Member

ovflowd commented Jan 6, 2025

Also, @flakey5 this https://github.com/syntax-tree/estree-util-visit is what I was referring to use to walk through the Nodes of the source code. Since acorn generates an AST that is compatible with estree.

@flakey5 flakey5 marked this pull request as ready for review January 15, 2025 23:55
@ovflowd
Copy link
Member

ovflowd commented Jan 26, 2025

@flakey5 there are PR conflicts. Could you fix'em?

src/parser.mjs Outdated Show resolved Hide resolved
Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far looking good. Left more comments, will review more once they are addressed-

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @flakey5 friendly bump :D

@flakey5 flakey5 force-pushed the flakey5/152 branch 3 times, most recently from 66affd4 to 355e992 Compare February 3, 2025 01:38
@flakey5 flakey5 requested a review from ovflowd February 3, 2025 20:43
src/index.mjs Outdated Show resolved Hide resolved
Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Left minor comments but the work you've done here is impressive and surely should be remarked.

Closes #152

Signed-off-by: flakey5 <[email protected]>

Co-authored-by: Claudio W <[email protected]>
Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT !

@flakey5 flakey5 requested a review from ovflowd February 7, 2025 20:03
Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think code sat long enough here. Whilst I believe some pieces can still be broken down even further (there are some huge switch statements within some methods that I believe can be moved to dedicated functions and even different files)... this is LGTM and we should get going to unblock our own work!

We can always iterate on this, cc @flakey5

Anyhow, just wanted to point out the amazing work you've done here!

@flakey5 flakey5 merged commit 11a7953 into main Feb 11, 2025
7 checks passed
@flakey5 flakey5 deleted the flakey5/152 branch February 11, 2025 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

urgent: feat: add apilinks.json generator
3 participants