Add neon::types::JsMap

[touilleMan]

close #1072

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]

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 {
        /* ... */
    }
}

[kjvalencik]

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.

[touilleMan]

Done, I've also added a dedicated test

[kjvalencik]

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

[touilleMan]

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

[kjvalencik]

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.

[kjvalencik]

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

[touilleMan]

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 😄

[kjvalencik]

Yep, exactly!

[kjvalencik]

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

[kjvalencik]

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]

@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]

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...)