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

Adds some documentation to public facing classes and functions #106

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

PlaryWasTaken
Copy link

Pretty much described in the title, adds some documentation, mainly for public facing functions and classes, i would have typed the options of factory.ts - createEngine function, but i don't understand the project enough. I did use AI to help me write some of these types

@ceifa
Copy link
Owner

ceifa commented Jan 25, 2024

There is some lint errors, can you fix them?

@ceifa
Copy link
Owner

ceifa commented Jan 25, 2024

Also I think it's a little bit weird to use TS with JSDoc, is it common in libraries?

@PlaryWasTaken
Copy link
Author

PlaryWasTaken commented Jan 25, 2024

It provides better description on what a function does
image
Example above in a Jetbrains IDE, Typescript does add a type system but you don't have any actual documentation when you only use TS, only type info.
Fixed linting with npm run lint, tested with npm run lint:no-fix no errors returned

@PlaryWasTaken
Copy link
Author

PlaryWasTaken commented Jan 25, 2024

Having these descriptions helps to aid in ambiguity where you might have a certain idea of how something works but you're not sure, thats why i also wanted to type the options in createEngine, but im not sure what every one of those options does under the hood

@PlaryWasTaken
Copy link
Author

Oh, i forgot that tsconfig has "removeComents" as true, for JSDoc to be able to be used for this "removeComments¨ has to be false, is there any reason for this to be set to true?

@ceifa
Copy link
Owner

ceifa commented Jan 27, 2024

Oh, i forgot that tsconfig has "removeComents" as true, for JSDoc to be able to be used for this "removeComments¨ has to be false, is there any reason for this to be set to true?

Some libraries have comments on their code, this option removes it to decrease the bundle size.

@PlaryWasTaken
Copy link
Author

Also I think it's a little bit weird to use TS with JSDoc, is it common in libraries?

Well, mongoose uses them, and i've seen some other libs use them too

@ceifa
Copy link
Owner

ceifa commented Feb 4, 2024

Well, mongoose uses them, and i've seen some other libs use them too

Mongoose uses JS + JSDoc...

Just found out there is a TSDoc: https://tsdoc.org/

@PlaryWasTaken
Copy link
Author

Well, mongoose uses them, and i've seen some other libs use them too

Mongoose uses JS + JSDoc...

Just found out there is a TSDoc: https://tsdoc.org/

Well, i tried to use mongoose as an example since it was the first one that came to mind, and it was a bad example, so im sorry.
Typescript does support JSDoc, and TSDoc is the same as JSDoc but a reduced subset
https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html
I will convert my JSDoc comments to the reduced subset that typescript accepts

… original method of extracting from the constructor gave any types on the options, this adds explicit typing and documents 2 options
@ceifa
Copy link
Owner

ceifa commented Feb 29, 2024

I would like these comments to be only on the .d.ts files, to not affect the bundle size: microsoft/TypeScript#14619

But since this is not something on TS yet, I just have to check how this affects our bundle and it's good to merge.

src/factory.ts Outdated Show resolved Hide resolved
public doString(script: string): Promise<any> {
return this.callByteCode((thread) => thread.loadString(script))
}

/**
* Executes Lua code from a file asynchronously.
* @async
Copy link
Owner

Choose a reason for hiding this comment

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

No need to add @async with TSDoc

Copy link
Author

Choose a reason for hiding this comment

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

Forgot to remove async

Copy link
Owner

Choose a reason for hiding this comment

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

please just remove this async and we are good to merge :)

src/engine.ts Outdated Show resolved Hide resolved
src/global.ts Show resolved Hide resolved
src/factory.ts Show resolved Hide resolved
src/types.ts Show resolved Hide resolved
src/types.ts Show resolved Hide resolved
src/types.ts Show resolved Hide resolved
@PlaryWasTaken
Copy link
Author

Ill get to it soon, thanks for helping with the comments for CreateEngineOptions
Also there is a workaround to only adding comments in declaration files in the issue you mentioned, tough it requires a change in the build scripts and i havent tested it yet

          A sane workaround:
tsc --removeComments && tsc --declaration --emitDeclarationOnly

With tsconfig.json options:

"removeComments": false,
"declaration": false,

Builds stripped JS, then emits declarations with JSDoc preserved.

Originally posted by @michal-kapala in microsoft/TypeScript#14619 (comment)

@RodrigoDornelles
Copy link

@ceifa would like to have access to more docs, can I continue new PR just by making the remaining adjustments?

indded, what do you think about me preparing a workflow with github-pages and some documentation generator tool? jsdoc, doxygen, something like that...?

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