infinyon / node-bindgen

Easy way to write Node.js module using Rust
Apache License 2.0
507 stars 43 forks source link

Support of options #278

Closed DmitryAstafyev closed 9 months ago

DmitryAstafyev commented 9 months ago

Issue

Looks like supporting of option type (Option<T>) isn't fully implemented.

Rust code

use node_bindgen::derive::node_bindgen;

#[node_bindgen]
fn test(a: Option<i32>, b: Option<i32>) -> i32 {
    a.unwrap_or(b.unwrap_or(1))
}

On JS side I'm expecting to do as well:

console.log(nativeModuleRef.test(100,200)); // OK
console.log(nativeModuleRef.test(200)); // OK
console.log(nativeModuleRef.test(null, 300)); // FAIL
console.log(nativeModuleRef.test(200, null));// FAIL
console.log(nativeModuleRef.test(undefined, 300)); // FAIL
console.log(nativeModuleRef.test(undefined, undefined));// FAIL
console.log(nativeModuleRef.test()); // OK

For example from serde_json perspective Option::None equal to undefined and null as well. Parsing goes without issues and with undefined and with null and without definition of value on js-object.

Reproducing

I'm also attaching sources of working example.

# unpack
cd node-bindgen_issue_options
nj-cli build --release
node ./dist/lib.js

Suggestion

Value Option::None should have an equivalent on JS side - null and undefined as well. "From" rust world to JS world Option::None should be returned as null. Well in general my suggestion just to fit serde_json

node-bindgen_issue_options.zip

Many thanks in advance for your feedback.

DmitryAstafyev commented 9 months ago

@sehz please take a look: https://github.com/infinyon/node-bindgen/pull/280

DmitryAstafyev commented 9 months ago

@sehz somehow tests on mac are failed... all of them... even linux is green. It doesn't look like it related to suggested changes... Could you please join also and help with tests?

sehz commented 9 months ago

it probably that it is using old nodejs version. @morenol can you update that test?

morenol commented 9 months ago

It looks like it is happening on main too, because this PR is also failing https://github.com/infinyon/node-bindgen/pull/281

DmitryAstafyev commented 9 months ago

@sehz @morenol PR is green... looks like you fixed macos )

DmitryAstafyev commented 9 months ago

I guess it can be closed as well.