Page MenuHomePhabricator

Implement CommConstants HostObject with `NATIVE_MESSAGE_TYPES` property in C++
ClosedPublic

Authored by marcin on Jun 2 2023, 3:04 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Apr 14, 1:58 PM
Unknown Object (File)
Sun, Apr 14, 1:58 PM
Unknown Object (File)
Sun, Apr 14, 1:58 PM
Unknown Object (File)
Sun, Apr 14, 1:58 PM
Unknown Object (File)
Sun, Apr 14, 1:58 PM
Unknown Object (File)
Sun, Apr 14, 1:58 PM
Unknown Object (File)
Sun, Apr 14, 1:54 PM
Unknown Object (File)
Mar 5 2024, 3:49 PM
Subscribers

Details

Summary

This differential implements CommValidationModule validateMessageTypes method.

Test Plan

Build the app. We don't have a way to call this method yet.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

marcin requested review of this revision.Jun 2 2023, 3:21 AM

Replace TurboModule with HostObject

native/cpp/CommonCpp/NativeModules/CommConstants.cpp
20 ↗(On Diff #27441)
NOTE: The lack of caching here is intentional. jsi::Array happens to have deleted copy constructor. Theoretically we could use std::move here safely since according to jsi.h (I looked om it in the XCode) the move constructor for jsi::Array is default (so it acts as copy constructor). In practice it is better not to reuse objects that were once moved. We could also keep jsi::Array as an instance variable and manually copy its items to newly created jsi::Array that is returned here but I don't think it would be significantly faster than to recreate this array from scratch.
marcin retitled this revision from Implement CommValidationModule validateMessageTypes method in C++ to Implement CommConstants HostObject with `NATIVE_MESSAGE_TYPES` property in C++.Jun 6 2023, 2:39 AM
bartek added inline comments.
native/cpp/CommonCpp/NativeModules/CommConstants.cpp
10–12 ↗(On Diff #27441)

Doesn't this work?

20 ↗(On Diff #27441)

Yeah, in this case it makes sense - NATIVE_MESSAGE_TYPES probably will be accessed once, during the check (correct me if I'm wrong).

If we ever really need caching, we can do something that expo does for caching module objects (1, 2). Maybe it's worth adding a TODO comment or linear task? You can also try doing this right away if it doesn't take you too long

It's also worth noting that caching JSI objects requires proper deallocation - the cache should be cleared in HostObject's destructor or otherwise the app will crash (e.g. on reload)

native/cpp/CommonCpp/NativeModules/CommConstants.h
11 ↗(On Diff #27441)

I think this can be static or outside the class, but up to you

This revision is now accepted and ready to land.Jun 6 2023, 2:41 AM

Implement constants caching

This is cool – our first HostObject!