nitro icon indicating copy to clipboard operation
nitro copied to clipboard

feat: add typed sets to supported Nitro types

Open Killavus opened this issue 6 months ago • 9 comments

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.

Killavus avatar Jun 14 '25 22:06 Killavus

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

vercel[bot] avatar Jun 14 '25 22:06 vercel[bot]

Hi - thanks for your PR, but I cannot merge as-is because;

  1. You applied different formatting. Is it your IDE or VSCode?
  2. There is no implementation for Kotlin or Swift, even tho you added it to the type docs table

mrousavy avatar Jun 15 '25 13:06 mrousavy

  1. Yeah, it was Android Studio doing the formatting. I'll reformat using clang-format as the chore commit.
  2. 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.

Killavus avatar Jun 15 '25 16:06 Killavus

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)

mrousavy avatar Jun 16 '25 10:06 mrousavy

I was thinking about this, but in my case:

  1. 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.
  2. The object needing a set-like getter is returned from one of the HybridA methods createB(): HybridB. HybridB is 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?

Killavus avatar Jun 16 '25 15:06 Killavus

ok I get it. and you absolutely need Set? Normal array doesn't work?

mrousavy avatar Jun 16 '25 16:06 mrousavy

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.

mrousavy avatar Jun 16 '25 17:06 mrousavy

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!

Killavus avatar Jun 17 '25 07:06 Killavus

Why not just .contains() instead of .has()

mrousavy avatar Jun 17 '25 09:06 mrousavy

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!

mrousavy avatar Dec 10 '25 17:12 mrousavy