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

WIP: add typings #112

Closed
wants to merge 17 commits into from
Closed

WIP: add typings #112

wants to merge 17 commits into from

Conversation

sandangel
Copy link

@sandangel sandangel commented May 21, 2018

Ref #106

package.json Outdated
},
"dependencies": {
"@mischnic/async-hooks": "^0.0.4",
"autogypi": "^0.2.2",
"libui-download": "^1.1.0",
"nbind": "^0.3.14",
"node-gyp": "^3.3.1"
}
},
"main": "./index.js",
Copy link
Owner

Choose a reason for hiding this comment

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

This is already the default for node, there is no need to insert this property

package.json Outdated
"proc-stats": "0.0.4"
"prettier": "^1.12.1",
"proc-stats": "0.0.4",
"typescript": "^2.8.3"
Copy link
Owner

Choose a reason for hiding this comment

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

Why we need typescript as a devdep? It will allow us to check types in example, tests etc?
If so, could you add an entry in scripts to do this check?

index.d.ts Outdated
type: textUnderlineColor,
color: Color): FontAttribute;

static getFamily(): textAttributeType|null;
Copy link
Owner

Choose a reason for hiding this comment

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

This is a string

index.d.ts Outdated
static getOTFeatures(): OpenTypeFeatures|null;
}

export class OpenTypeFeatures {
Copy link
Owner

Choose a reason for hiding this comment

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

This class has also add, remove, forEach methods

index.d.ts Outdated
static get(str: string): number|null;
}

export class Color {
Copy link
Owner

Choose a reason for hiding this comment

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

For Color, Point, and their double counterparts: number is ok, but they also has properties with the same names as the constructor arguments

Copy link
Author

Choose a reason for hiding this comment

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

@parro-it Sorry I can not get your point. Could you explain with some code?

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I think I get your point

Copy link
Owner

Choose a reason for hiding this comment

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

const p = new Point(12, 13);
console.log(p.x, p.y)

package.json Outdated
"ava": "^0.25.0",
"clang-format": "^1.2.2",
"humanize": "0.0.9",
"husky": "^0.14.3",
"proc-stats": "0.0.4"
"prettier": "^1.12.1",
Copy link
Owner

Choose a reason for hiding this comment

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

Why prettier? We already use clangformat to check style, I think prettier would clash with that: it probably format the code in a way that clangformat does not accept

Copy link
Author

Choose a reason for hiding this comment

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

I use prettier because it will automatically add ';', clang-format does not

Copy link
Author

Choose a reason for hiding this comment

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

before submit, I will run lint to make sure code will be formatted with clang-format

Copy link
Owner

Choose a reason for hiding this comment

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

I use prettier because it will automatically add ';', clang-format does not

Yes, and I also miss some other checks that was provided by eslint, but clangformat has the advantage that also format objective-c c and c++. We'll need to improve on this. Meanwhile, you can jsut install it globally?

Copy link
Author

Choose a reason for hiding this comment

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

OK, I see.

@sandangel
Copy link
Author

sandangel commented May 24, 2018

I have finished adding type to index.js. You can try adding //ts-check on top of the JS file and hover the class signature to see type information pop up in vscode.

About the UI part, I have read through the c++ code. I need some of your comments on which is the public API, how to use it, parameters information... This will help me work faster instead of having to switching between docs and code... I believe there is tons of tools to help you auto generate code based on comments so it will help keep the docs in sync as well.

Please add those and push to the 0.3.0 branch then notify me when you have finished.

@parro-it
Copy link
Owner

I have finished adding type to index.js. You can try adding //ts-check on top of the JS file and hover the class signature to see type information pop up in vscode.

Thank you, I'll try and confirm if it works.

About the UI part, I have read through the c++ code. I need some of your comments on which is the public API, how to use it, parameters information... This will help me work faster instead of having to switching between docs and code... I believe there is tons of tools to help you auto generate code based on comments so it will help keep the docs in sync as well.

You can check the JavaScript classes contained in https://github.com/parro-it/libui-napi/tree/master/js folder.

libui-napi was rewritten almost from scratch by me and @mischnic to use the new Node.js N-API functions and simplify prebuilding of binaries. It will be merged back in libui-node in version 0.4.0

The public API will be the same as now, and that classes map current C++ classes one - by - one.
They all have jsdoc annotation, used to generate the documentation you can find here: https://parro-it.github.io/libui-napi/#/menu.

If you prefer, and you think it's simpler, you can open a PR on libui-napi repo and insert TS directly there.

@sandangel
Copy link
Author

@parro-it thank you for pointing me to that library, it helps. I will continue working on this

@parro-it
Copy link
Owner

Hovering over function in VS Code works well, displaing corrects TS signatures.

@sandangel
Copy link
Author

@parro-it after I have finished this, you could simply copy all the jsdoc to the ts declare file so user don't have to go to docs page all the time ^^

@sandangel sandangel closed this Sep 14, 2019
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