-
Notifications
You must be signed in to change notification settings - Fork 595
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
add typing to most examples. #538
base: main
Are you sure you want to change the base?
Conversation
I'm not sure we want to add types to all of the examples, although I could see some examples with types would be good. @UrielCh just wondering on your maint maint motiviation in terms of adding the types? For people using TypeScript, or something else? |
In this case, having TypeScript typings for all the samples makes it easier to find examples of how to implement certain things. By adding types, I noticed in the sample that:
and
do not implements the same binding. export class MyObject{
public value: number;
constructor(num: number);
public plusOne(): number;
public multiply(num?: number): MyObject;
} Vs export declare class MyObject{
constructor(num: number);
public value(): number;
public plusOne(): number;
public multiply(num?: number): MyObject;
}
By the way I did not find any sample of binding a static method from a object. I'm trying to port opencv4nodejs to N-API as @u4/opencv4nodejs update:this issue is now addressed by the PR #539 |
other ... strange code in the but by adding a typing the binding.js can be simply replace by a typing. original: const addon = require('../build/Release/object-wrap-demo-native');
function ObjectWrapDemo(name) {
this.greet = function(str) {
return _addonInstance.greet(str);
}
var _addonInstance = new addon.ObjectWrapDemo(name);
}
module.exports = ObjectWrapDemo; have the same effect that: /**
* @type {import('./type')}
*/
const addon = require('../build/Release/object-wrap-demo-native');
module.exports = addon.ObjectWrapDemo; once the typing: export class ObjectWrapDemo {
constructor(name: string);
/**
* return a string like "Hello, ${str}\nI am ${name}\n"
* @param str greeting string
*/
greet(str: string): string;
} is added. |
We discusssed in the node-api team meeting today and the consensus seems to be that adding the jsdoc/types info is not helpfull and might distract from the example itself. Instead working on adding additional typescript examples or improving the existing typescript example - https://github.com/nodejs/node-addon-examples/tree/main/src/8-tooling/typescript_with_addon/node-addon-api would would be beneficial. Maybe even using the new experimental feature in Node.js to strip out types and run typescript directly. |
Adding a sample to show how to use typing, and how to use experimental TS support in nodeJS 22+, why not, but what code should I implement in the Native code ? a simple return "hello World" ? a class ? Each sample adds some native code complexity. Can you please check my PR: #539 The 3 code samples should implement the same interface but thy do not, and I notice that by adding typing to this sample. |
The suggestion was that updating the existing typescript example to be how the workflow is like, using I don't think this needs to update the native code. |
those change add typing to most examples.