-
Notifications
You must be signed in to change notification settings - Fork 176
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
[apiview-js-parser] migrate to the new tree token #9034
base: main
Are you sure you want to change the base?
[apiview-js-parser] migrate to the new tree token #9034
Conversation
- build output to ./dist - add eslint, prettier - add v2 generation side-by-side in generate.ts and jstokens.ts
and build release tags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an awesome improvement - and I like the way you structured the code. I only have two asks (neither is blocking)
- Would you mind adding some documentation: either a md file with your understanding of the flow and how APIView gets generated (kinda like an architecture.md doc) or just doc-comments throughout the code
- Any chance we can get a screenshot or gif? I am very curious what the new stuff looks like
My only reasoning for (1) is that this stuff looks complex, and the old code was complex (took me a while to even understand it!), so it would be great to get all the context you picked up out of your head and onto paper while it's still fresh
} | ||
} | ||
|
||
let name = apiModel.packages[0].name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can have a helper function that encapsulates the building of name
+ the logic to get the version from the filename? It'll just be easier understand it
It could even be const { packageName, packageVersion } = parseFileName(fileName)
or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name is from apiModel so it is not just from fileName, but I tried some refactoring.
const dependencies = apiJson.metadata.dependencies; | ||
|
||
const result = JSON.stringify( | ||
GenerateApiview({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am so sorry for the nitpicky comment but functions are usually camelCase 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not know why it is capitalized!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
|
||
function buildDocumentation(reviewLines: ReviewLine[], member: ApiItem, relatedTo: string) { | ||
if (member instanceof ApiDocumentedItem) { | ||
const lines = member.tsdocComment?.emitAsTsdoc().split("\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this add doc-comments to the APIView? 😍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes based on my understanding. It will add whatever ref docs included in the api.json. By default it is hidden.
import jsTokens from "js-tokens"; | ||
import { type ReviewToken, TokenKind } from "./models"; | ||
|
||
const TS_KEYWORDS = new Set<string>([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did this list come from and what does it impact? Just colorization? Or something else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should link to it in a comment https://github.com/microsoft/TypeScript/blob/aa9df4d68795052d1681ac7dc5f66d6362c3f3cb/src/compiler/scanner.ts#L135
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I think so, colorization for keywords in apiview.
Good point! I will add a README.md
I have not seen it in the UI either, hopefully soon though |
- refactoring
- refactoring - add ref docs
- replace "export declare function" with "export function"
Migrate to the new tree token based scheme. While at it, also