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: support for JSDoc extraction #248

Draft
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

GalacticHypernova
Copy link

@GalacticHypernova GalacticHypernova commented Jun 16, 2023

This PR aims to extend the auto import process to allow for automatic JSDoc extraction for better in-IDE documentation.
Initially thought of using for nuxt, I figured this might benefit all tools that may use unimport.

@GalacticHypernova GalacticHypernova marked this pull request as draft June 16, 2023 15:00
@GalacticHypernova
Copy link
Author

GalacticHypernova commented Jun 16, 2023

I am marking this as draft for now, until I make sure it all works well, since a couple issues have risen from my initial testing. Also, yes, I know my code isn't the best, as this is the prototype version. Any constructive criticism and suggestions for improvement are appreciated!

@GalacticHypernova GalacticHypernova changed the title Experimental auto JSDoc extraction feat: Experimental auto JSDoc extraction Jun 16, 2023
@GalacticHypernova GalacticHypernova changed the title feat: Experimental auto JSDoc extraction feat: Automatic JSDoc extraction Jun 16, 2023
@GalacticHypernova GalacticHypernova marked this pull request as ready for review June 17, 2023 15:18
@GalacticHypernova
Copy link
Author

Marking this as ready for review after all testing I could do was done, would appreciate feedback!

src/utils.ts Outdated Show resolved Hide resolved
@GalacticHypernova GalacticHypernova marked this pull request as draft June 20, 2023 15:36
@GalacticHypernova
Copy link
Author

GalacticHypernova commented Jul 1, 2023

I migrated from TypeScript, in stead I used regex patterns, but this was a big change so I may or may not have broken something. Would appreciate feedback!
It seems to work on my local machine for any exposed function (and const) with a valid JSDoc annotation.

@GalacticHypernova GalacticHypernova marked this pull request as ready for review July 1, 2023 18:05
@GalacticHypernova GalacticHypernova marked this pull request as draft September 22, 2023 17:56
@GalacticHypernova GalacticHypernova marked this pull request as ready for review September 22, 2023 17:57
@GalacticHypernova
Copy link
Author

GalacticHypernova commented Oct 5, 2023

@antfu sorry for being impatient, but is there any updates on this? I do think this will greatly improve overall DX.

@GalacticHypernova
Copy link
Author

@pi0 is there anything else I should include/change?

@GalacticHypernova
Copy link
Author

GalacticHypernova commented Oct 20, 2023

@danielroe I forgot to ask, should I make a test case for this? I'm not the best at writing tests (in fact, I have never done it) but I can certainly try if it's needed, I'm just not sure what the conditions for writing tests are.

@antfu
Copy link
Member

antfu commented Oct 23, 2023

I think, in general, the quality of this PR does not match the standard:

  • Hard-coded specific handling (despite the review comment, the issue still presets)
  • The solution seems to be quite hacky (relies on conventions rather than a proper resolving algorithm)
  • There are no tests to guarantee how it would work.
  • It doesn't follow the repo's linting and formatting rules.

I am sorry for taking long to respond. I think the feature itself is legit and would indeed benefit the DX if we implement it well. However, we would need time to think of a more robust solution.

@GalacticHypernova
Copy link
Author

I'll return this to a draft again while I make some more tests

@GalacticHypernova GalacticHypernova marked this pull request as draft October 23, 2023 04:51
@GalacticHypernova
Copy link
Author

There is a slight issue. I don't think there's a way to make this more framework agnostic, mainly because of the existent changes between framework. Yes, this is based on conventions, and yes, it's not really agnostic. But it's reliable, and it catches all the edge cases. How would you go about making it not rely on conventions?

@GalacticHypernova GalacticHypernova marked this pull request as ready for review January 22, 2024 21:07
@GalacticHypernova
Copy link
Author

GalacticHypernova commented Jan 22, 2024

Attempt number 3, I have completely reworked the way I handle this, and now I am utilizing map and reading the files to rely as little as possible on conventions. When the file isn't found through package.json it checks all extension, otherwise will just return null. Would love to hear your opinion!

@GalacticHypernova GalacticHypernova changed the title feat: Automatic JSDoc extraction feat: support for JSDoc extraction Jan 24, 2024
@GalacticHypernova GalacticHypernova marked this pull request as draft January 24, 2024 22:08
@GalacticHypernova
Copy link
Author

Converting to a draft to work on the discussed implementation.

@pi0
Copy link
Member

pi0 commented Jan 25, 2024

Sorry i will try to be back on this as soon as I can (probably next week, sorry again).

For the digest, we have talked with @antfu and @GalacticHypernova to use of an AST parser to extract JSDocs. untyped already exposes a bundled AST parser as well as tools that allow this. However thinking more, since mlly also plans to switch to AST parsers (unjs/mlly#219) + the fact that we probably don't want to make users (of all these packages) have multiple versions of AST parser, I think probably best course of action would be wait for AST switch in mlly + try to use babel parser (which supports TypeScript) or untyped for making a full proof on concept here in this PR.

Alternatives that probably not work:

  • Regexp: Lots of edge cases
  • TS Server: Huge dependency and also lacks support for just giving JSDocs even if wan to
  • Acorn parser: ESM Only no typescript support
  • Wasm parser: Nice (and something I aim to universally migrate to across UnJS) but probably to early and risky for this POC because it can cause diversion and installing native dep is tricky specially if multiple packages do it.

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.

3 participants