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

Support of options #278

Closed
DmitryAstafyev opened this issue Nov 15, 2023 · 6 comments
Closed

Support of options #278

DmitryAstafyev opened this issue Nov 15, 2023 · 6 comments
Labels
enhancement New feature or request

Comments

@DmitryAstafyev
Copy link
Contributor

DmitryAstafyev commented Nov 15, 2023

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
Copy link
Contributor Author

@sehz please take a look: #280

@DmitryAstafyev
Copy link
Contributor Author

@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
Copy link
Collaborator

sehz commented Nov 17, 2023

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

@morenol
Copy link
Contributor

morenol commented Nov 17, 2023

It looks like it is happening on main too, because this PR is also failing #281

@DmitryAstafyev
Copy link
Contributor Author

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

@sehz sehz added enhancement New feature or request and removed help wanted Extra attention is needed labels Nov 17, 2023
github-merge-queue bot pushed a commit that referenced this issue Nov 17, 2023
* Consider undefined/null for optional rust types

* Fix version of dependency (nj-core)

* Add test
@DmitryAstafyev
Copy link
Contributor Author

I guess it can be closed as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants