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

add typing to most examples. #538

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

add typing to most examples. #538

wants to merge 4 commits into from

Conversation

UrielCh
Copy link

@UrielCh UrielCh commented Sep 9, 2024

those change add typing to most examples.

@mhdawson
Copy link
Member

mhdawson commented Sep 9, 2024

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?

@UrielCh
Copy link
Author

UrielCh commented Sep 9, 2024

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:

  • node-addon-examples/src/1-getting-started/6_object_wrap/napi

and

  • node-addon-examples/src/1-getting-started/6_object_wrap/nan
  • node-addon-examples/src/1-getting-started/6_object_wrap/node-addon-api

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;
}

value vs value()

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

@UrielCh
Copy link
Author

UrielCh commented Sep 9, 2024

other ... strange code in src/2-js-to-native-conversion/object-wrap-demo/node-addon-api with the extra typing:

the binding.js is wrapper to a native class: ObjectWrapDemo

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.

@mhdawson
Copy link
Member

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.

@UrielCh
Copy link
Author

UrielCh commented Sep 28, 2024

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.

@legendecas
Copy link
Member

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 ?

The suggestion was that updating the existing typescript example to be how the workflow is like, using tsc, or using the type stripping flag, and how to provide type information for an addon .node file.

I don't think this needs to update the native code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Need Triage
Development

Successfully merging this pull request may close these issues.

3 participants