Add neon::types::JsMap #1080

touilleMan · 2024-10-11T12:01:28Z

This PR is not done yet, but I'd like some feedback on the implementation:

I had to add instanceof which wasn't currently defined in the NAPI1. I think it would be good to also expose this to the end users (maybe cx.instanceof() ?), typically when I first discovered Neon doesn't provide Map bindings I tried to implement it myself in my codebase... only to find instanceof wasn't available as well ! 😄
My implementation of sys::tag::is_map is very low level and clumsy, I feel like their should be a cleaner way to implemented it ^^
Map.groupBy is currently broken (this is the added test that is not passing), the issue comes from a failed to downcast any to object error when trying to downcast Map.groupBy into a JsFunction. I guess this is related to the fact groupBy is a static method, but I don't know how to deal with it (maybe entirely bypassing JsFunction and using lower level code instead ?).
What should be the policy for return types ? For instance when a method is expected (according the MDN documentation) to return undefined should we check the actual returned value is undefined then return an empty tuple to stick with the Rust way, or should we just avoid any guesswork and return a JsValue no matter what ?
I'm unsure if and how to implement Map[Symbol.species] and Map.prototype[Symbol.iterator]()

(FYI I've put TODO about those points in the code)

kjvalencik

Thanks so much for this contribution @touilleMan! It'll be really great to have this type in here.

In terms of the design, I would like to have more ergonomic wrappers (i.e., that take Rust types). Unfortunately, this makes the design space a lot more complicated.

What is the minimum set of features you need for your use case? Maybe we could initially start with a smaller version of Map that doesn't expose everything and we could iterate in future PRs.

kjvalencik · 2024-10-11T13:25:39Z

crates/neon/src/sys/bindings/functions.rs

@@ -51,6 +51,9 @@ mod napi1 {

            fn close_handle_scope(env: Env, scope: HandleScope) -> Status;

+            fn instanceof(env: Env, object: Value, constructor: Value, result: *mut bool)


This is awesome! Thanks for adding it. I think we should add it as a method on the Value trait.

trait Value { fn instance_of<V: Value>(&self, cx: &mut Cx, ctor: &V) -> bool { /* ... */ } }

Would you mind opening a separate PR to implement this? It's a great feature and we can get it merged pretty quick.

I'm happy to pick it up if you prefer.

Done, I've also added a dedicated test

kjvalencik · 2024-10-11T13:26:32Z

test/napi/src/lib.rs

@@ -233,6 +234,24 @@ fn main(mut cx: ModuleContext) -> NeonResult<()> {
    cx.export_function("return_js_array_with_string", return_js_array_with_string)?;
    cx.export_function("read_js_array", read_js_array)?;

+    cx.export_function("return_js_map", return_js_map)?;


Can we use the #[neon::export] macro for all of the new tests?

Yes ! that's much cleaner this way \o/

crates/neon/src/sys/tag.rs

kjvalencik · 2024-10-11T13:33:01Z

crates/neon/src/sys/tag.rs

@@ -137,3 +139,37 @@ pub unsafe fn check_object_type_tag(env: Env, object: Local, tag: &super::TypeTa
 pub unsafe fn is_bigint(env: Env, val: Local) -> bool {
    is_type(env, val, napi::ValueType::BigInt)
 }
+
+pub unsafe fn is_map(env: Env, val: Local) -> bool {


In my opinion, there isn't a great reason to put this in sys. A lot of the sys patterns like sys::tag go back to when Neon supported a C++ backend (prior to Node-API).

The current conventions are to put this code alongside the safe implementation. This also makes it easier to use other safe wrappers instead of falling back to unsafe. Less unsafe code is needed. Unfortunately, the ValueInternal trait uses pointers, so some unsafe is needed, but only for getting a Context and Handle.

I opened #1081 to allow implementing ValueInternal::is_typeof without any unsafe.

So you mean I should entirely remove sys::tag::is_map and instead put it content directly inside impl private::ValueInternal for JsMap's is_typeof ?

EDIT: I've removed sys::tag::is_map and implemented in JsMap::is_typeof it's very satisfying how simpler it makes the code 😄

Yep, exactly!

crates/neon/src/sys/tag.rs

kjvalencik · 2024-10-11T13:37:59Z

crates/neon/src/types_impl/map.rs

+            .construct_with(cx)
+            .apply::<JsObject, _>(cx)?;
+
+        Ok(map.downcast_or_throw(cx)?)


FYI, downcasting is necessary. Since it's possible to override global.Map, we don't know that it's actually the built-in. If someone were to override it, the constructor might return a different type.

global.Map = function Map() { return new Array(); }; new Map() instanceof Map // false

crates/neon/src/types_impl/map.rs

kjvalencik · 2024-10-11T21:00:16Z

My implementation of sys::tag::is_map is very low level and clumsy, I feel like their should be a cleaner way to implemented it ^^

Unfortunately, Map isn't in Node-API so it does require the clunky grabbing Map off of the global object and instance checking. This is true about many of the types that are implemented is globals and not built into the language.

I do think we can clean it up a bit by implementing with the safe API wrappers and implementing LocalKey<Root<_>> cached lookups.

touilleMan · 2024-10-11T21:29:01Z

@kjvalencik Thanks for you precise and quick feedback, it's a very motivating to have such nice interactions when submitting a PR 🎉

I've pushed a new comment that should have addressed all your comments (don't hesitate to tell me if you think more changes are needed ^^)

(Once the PR is ready for merging, I will squash the commits together to keep things clean)

What is the minimum set of features you need for your use case? Maybe we could initially start with a smaller version of Map that doesn't expose everything and we could iterate in future PRs.

I'm doing simple conversion between Rust HashMap and Javascript Map. So my needs are really minimal: just being able to create a map and get/set items.

In terms of the design, I would like to have more ergonomic wrappers (i.e., that take Rust types). Unfortunately, this makes the design space a lot more complicated.

What do you have in mind ? Having keys/values methods returning a wrapper over the javascript iterator object ?

touilleMan · 2024-10-11T21:48:19Z

Also regarding Map.groupBy, I've added the following debug code in my JSMap::group_by method:

        let xx = cx.string("console.log('Map.groupBy:', Map.groupBy)");
        crate::reflect::eval(cx, xx)?;

And this prints undefined, so I guess this explain why JSMap::group_by has this failed to downcast any to object error.
So I guess the issue comes from the version of Node I'm using (i.e. 18.17.0) which is too old to have Map.groupBy (as it seems a rather recent addition to the standard)

What should be done then ? I would say doing self.method(cx, "groupBy")? in the codebase should detect that the method is not available and return the correct not found error, however it seems to accept the undefined value and the error only occurs later when a downcast is tried (which leads to the more cryptic error I was initial talking about...)

feat(neon): Add neon::types::JsMap

0d8969e

touilleMan force-pushed the js-map-object branch from 80343e9 to 0d8969e Compare October 11, 2024 12:07

kjvalencik reviewed Oct 11, 2024

View reviewed changes

kjvalencik mentioned this pull request Oct 11, 2024

chore(neon): Improve ValueInternal safety #1081

Merged

Address @kjvalencik comments

c1b1ee0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add neon::types::JsMap #1080

Add neon::types::JsMap #1080

touilleMan commented Oct 11, 2024 •

edited

Loading

kjvalencik left a comment

kjvalencik Oct 11, 2024

kjvalencik Oct 11, 2024

touilleMan Oct 11, 2024

kjvalencik Oct 11, 2024

touilleMan Oct 11, 2024

kjvalencik Oct 11, 2024

kjvalencik Oct 11, 2024

touilleMan Oct 11, 2024 •

edited

Loading

kjvalencik Oct 11, 2024

kjvalencik Oct 11, 2024

kjvalencik commented Oct 11, 2024

touilleMan commented Oct 11, 2024

touilleMan commented Oct 11, 2024 •

edited

Loading

		@@ -51,6 +51,9 @@ mod napi1 {

		fn close_handle_scope(env: Env, scope: HandleScope) -> Status;

		fn instanceof(env: Env, object: Value, constructor: Value, result: *mut bool)

Add neon::types::JsMap #1080

Are you sure you want to change the base?

Add neon::types::JsMap #1080

Conversation

touilleMan commented Oct 11, 2024 • edited Loading

kjvalencik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

touilleMan Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjvalencik commented Oct 11, 2024

touilleMan commented Oct 11, 2024

touilleMan commented Oct 11, 2024 • edited Loading

touilleMan commented Oct 11, 2024 •

edited

Loading

touilleMan Oct 11, 2024 •

edited

Loading

touilleMan commented Oct 11, 2024 •

edited

Loading