feat: add typed sets to supported Nitro types
This PR adds a new type recognized by Nitrogen (Set<T>) and provides a JSI conversion from JS sets into C++ std::unordered_set .
Why
There are certain Web APIs that are returning set-like objects like FontFaceSet and they are returning them from getters. Since there is no equivalent registerRawHybridGetter (only registerRawHybridMethod) available, these APIs were hard (impossible?) to express with Nitro.
I'm very happy to adjust the implementation - my C++ is not great, so any tips are welcome.
Also this PR breaks one invariant of JS Set - namely the insertion order. There may be some boost containers that allows you to upheld this, but I wanted to base my implementation on std::unordered_set for simplicity.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
| Name | Status | Preview | Comments | Updated (UTC) |
|---|---|---|---|---|
| nitro-docs | ✅ Ready (Inspect) | Visit Preview | 💬 Add feedback | Jun 14, 2025 11:02pm |
Hi - thanks for your PR, but I cannot merge as-is because;
- You applied different formatting. Is it your IDE or VSCode?
- There is no implementation for Kotlin or Swift, even tho you added it to the type docs table
- Yeah, it was Android Studio doing the formatting. I'll reformat using
clang-formatas the chore commit. - Tbh I could not find the place to implement and assumed there is some kind of autowrapping of C++ types done on both Swift and Kotlin side. I'll check the code and implement Swift & Kotlin impl as well.
I'll ping you once this is done.
Before you go this extra mile; are we sure we need fully native support for Set?
This has some overhead due to the JSI calls and JSI constructions (which are not cached).
Why not just array, which is natively supported? Then just drop duplicates or wrap it in a Set on JS side. Imo much cleaner and very likely also more performant (unless you hit a ton of duplicates)
I was thinking about this, but in my case:
- I have many hybrid objects in my library. I only auto-link one - let's call it
HybridA. It is a top-level object I construct as a singleton from JavaScript side. - The object needing a set-like getter is returned from one of the
HybridAmethodscreateB(): HybridB.HybridBis not auto-linked.
I assume in order to do do this I'd need to auto-link HybridB, create a dummy object in JS (using createHybridObject) to access its prototype, modify its prototype to provide a getter which wraps it in Set for future objects. Would it even work in case that C++ side is probably setting a prototype? And also I'd need to figure out how to modify TypeScript types to 'see' a getter from dynamically adjusted prototype... I can of course try, but I think having Set returned directly makes it cleaner - I get a 'proper' interface directly from .nitro.ts definition.
And the end goal for me is to be able to run code written for Web in RN, so even small API differences may result in someone not being able to run it in RN context without modifying code. I'll check if I can minimize JSI calls - I assume I should minimize callAsConstructor and callWithThis calls in order to make it performant?
ok I get it. and you absolutely need Set? Normal array doesn't work?
Maybe I can find a way to implement JS-based prototypes as C++ classes, basically the other way around of a HybridObject. I can then implement smarter caching for this. Let me think about this more, let's put this PR on pause for now - we need this anyways for Promise too.
ok I get it. and you absolutely need Set? Normal array doesn't work?
As long as user of my library won't need to use has() 😆 it will work! But unfortunately it's a common usage pattern.
let's put this PR on pause for now - we need this anyways for Promise too.
Sure. I'll prepare a PR to be mergable anyway in case it'll be needed once I find some free time. Thank you!
Why not just .contains() instead of .has()
appreciate the effort for this PR, but I think this type is not super necessary in core - a normal array will work fine for now, you can de-dupe in either JS or native anyways. thanks tho!