This differential introduces new JSI module to the project - CommValidationModule. Its intention is to enforce in JS things we assume about JS in C++ and vice versa. Currently it contains only one
method - validateMessageTypes, which aim is to encure all MessageSpecs implemented in JS are implemented in C++ as well.
Details
Details
- Reviewers
bartek
Run yarn codegen-jsi and ensure that nothing is changed in generated files.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- marcin/eng-1159
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
If you think it is an overkill to introduce new JSI module for this purpose then I am likely to agree with you. I hesitated between:
- new method in CommCoreModule
- custom jsi::HostObject implemented in C++ and exported to global and manually types in flow.
I didn't choose 1 since I feel like CommCoreModule is becoming to messy and we should be breaking the pattern of adding there whatever we feel like necessary.
I didn't choose 2 since I didn't like the idea where two general purpose modules are added to global along with some small custom object containing just one specific method. I wanted a layer of abstraction.
However if you feel it is not the right approach then request changes here and focus on child differential. It contains implementation of this method in C++ that will be used whichever solution we agree on.
Comment Actions
I didn't choose 1 since I feel like CommCoreModule is becoming to messy and we should be breaking the pattern of adding there whatever we feel like necessary.
Agree on that. CommCoreModule is already too large.
I didn't choose 2 since I didn't like the idea where two general purpose modules are added to global along with some small custom object containing just one specific method. I wanted a layer of abstraction.
Here I'd argue. This is because TurboModules are in fact just global objects containing methods, with C++ getters. They're not very different. Also the global space isn't reserved for turbomodules only, but I understand you don't want to bloat it too much.
- If you're exporting functions, then turbomodules should be considered because they do the property caching and other JS<->CPP glue for you
- If you're exporting custom objects (e.g. with dynamic properties) - subclassing HostObjects directly could be a better option because caching won't help you and you can avoid boilerplate (all codegen stuff).
From what I understood in our DM chat, you wanted to simply export an array of message types from C++ and do the validation logic in JS - I'm leaning towards this solution because it doesn't increase native code complexity and it's easier to maintain (JS-only changes, no need to rebuild etc). In that case, a separate turbomodule seems like an overkill.
But if the array is compile-time constant, maybe a good idea would be to add this to CommCoreModule because they're kind of business-logic constants and fit well if we could do something like commCoreModule.NATIVE_MESSAGE_TYPES. However, this pattern seems not to be supported by RN yet (without hacks) and you'd have to do commCoreModule.getConstants().NATIVE_MESSAGE_TYPES which is not very nice.
So my another idea is to create some CommConstants "module" which in fact isn't a TurboModule, but a HostObject that exports constants directly, without using getConstants(). It's simple and flexible (adding further constants) and purposeful (not bloating global for each constant separately, we can do global.CommNativeConstants.MESSAGE_TYPES). The disadvantage is that codegen won't work and flow types have to be added manually.
Another matter I'm considering right now is that this is the dev-only check. It's not worth having that much boilerplate (separate turbomodule + codegen) only for that, unless we want to validate more things in the future.
Sorry for chaotic message, too many considerations at once 😅 Requesting changes to get your opinion
Comment Actions
But if the array is compile-time constant, maybe a good idea would be to add this to CommCoreModule because they're kind of business-logic constants and fit well if we could do something like commCoreModule.NATIVE_MESSAGE_TYPES. However, this pattern seems not to be supported by RN yet (without hacks) and you'd have to do commCoreModule.getConstants().NATIVE_MESSAGE_TYPES which is not very nice.
This array is initialized at runtime but it is a global constant so theoretically commCoreModule.getConstants().NATIVE_MESSAGE_TYPES would work.
So my another idea is to create some CommConstants "module" which in fact isn't a TurboModule, but a HostObject that exports constants directly, without using getConstants(). It's simple and flexible (adding further constants) and purposeful (not bloating global for each constant separately, we can do global.CommNativeConstants.MESSAGE_TYPES). The disadvantage is that codegen won't work and flow types have to be added manually.
I think I would lean towards this solution. Taking account this issue: https://linear.app/comm/issue/ENG-3904/refactor-operations-approach-in-codebase, we plan to split CommCoreModule into separate classes located in separate files anyway, so implementing custom HostObject in a separate file fits nice into our future plans. Secondly, we can always just introduce a new method in CommCoreModule called getConstants that will simply return an instance of this object. Finally, my intuition is that implementing getConstants in CommCoreModule directly would require us to require us to return jsi::Value so we would need to manually type the return value of this function anyway.