Implement message spec interface for each type of message that mirrors corresponding JavaScript implementation.
Details
Details
No testing plan. Guture diffs will provide a code that was used on Android and iOS to parse raw message string and store it to database.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
This looks great to me, but @palys-swm should definitely take a look since I think he has some context on folly::dynamic
| native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageSpecs.h | ||
|---|---|---|
| 2 ↗ | (On Diff #12370) |
|
Comment Actions
One thing we should think about here: how do we make sure that somebody introducing a new message type remembers to create a new C++ MessageSpec?
Unrelatedly, we could consider writing these message specs in Rust, and trying to move more of the "business logic" of native to Rust.
Comment Actions
- Currently we do not implement MessageSpec for all type of messages. In particular you advised me during 1:1 not to implement it for messages that do no generate notifications (ADD_MEMBERS etc.). This way if message type is not present in MESSAGE_SPEC map we return nullptr as unique pointer to its content. Nevertheless we still have enumeration type that covers all message types. We can perform some simple check in translateRawMessageInfoToClientDBMessageInfo in next diff to throw runtime error if message of type not covered by this enumeration is received. If someone is led to MessageSpecs.h they will probably realise thet need to implement MessageSpec for that message if they want its content to be stored in the database. How is it currently done on the JS part? Perhaps we could apply the same solution here.
- I am open to writing those message specs in Rust, but I am not sure what exactly do you mean. We can:
- implement entire functionality of parsing notification in Rust.
- implement it twice (in C++ with higher priority and in Rust in the meantime)
- implement entire functionality in C++ and some part of it in Rust (for future work)
- Implement some part in C++ and some part in Rust and use binary bridges
Which one did you actually had in mind?
Comment Actions
Please, add the missing newlines.
| native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageSpecs.h | ||
|---|---|---|
| 22 ↗ | (On Diff #12402) | I think it's better to use enum class, can we use it here? |
| 44 ↗ | (On Diff #12402) | Why this name is upper-case-snake? It's not static. const for collections doesn't prevent modifying them (it prevents re-assigning). |
| 92 ↗ | (On Diff #12402) | I'm not sure about this. Is this going to create all these objects despite whether they are used or not? Maybe it would be better to create them lazily? Have you considered some kind of a factory here? |
| native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageSpecs.h | ||
|---|---|---|
| 44 ↗ | (On Diff #12402) | Actually const keyword prevents STL collections modification. But I agree with the rest of the comment that upper-case snake might not have been the best choice here. How about const std::map<MessageType, std::unique_ptr<MessageSpec>> messageSpecsHolder? |
| 92 ↗ | (On Diff #12402) | Yes, all this objects will be created when the app starts. There are solutions to create them lazily. folly::Lazy or folly::Singleton (it is also lazy and provides additional Singleton functionality) so I can implement their lazy instantiation. Nevertheless I am just curious about the potential drawbacks of eager instantiation. Those objects are stateless and there are just few of them so the memory overhead should not be an issue here. Still I agree I may be wrong and I am willing to work on lazy instantiation. |
| native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageSpecs.h | ||
|---|---|---|
| 44 ↗ | (On Diff #12402) | Looks better. Sorry, what I said was wrong about const, I confused this with the js where const prevents reassigning but you can still modify the collection. Nevertheless, we should use camel case. |
| 92 ↗ | (On Diff #12402) | For me, there are a lot of objects, not just a few. I'd lean towards the lazy init. It is a good rule of thumb let's say to avoid allocating memory as much as possible. If these objects ever grow, it will be harder to refactor this code to use the lazy init. |
Comment Actions
Pinging this question – I think it's important. Ideally the program crashes if there is a message type in JS that isn't in C++, and vice versa.
| native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageSpecs.h | ||
|---|---|---|
| 92 ↗ | (On Diff #12402) | I think eager initialization is fine for the message specs, personally. @palys-swm – what do you think? |
Comment Actions
@marcinwasowicz, sorry I missed your response earlier!
I think we'll want some protection to make sure somebody doesn't create a MessageSpec in JS with generatesNotifs: true without also creating a MessageSpec in C++. One thing we could do is do a check at app boot for every JS MessageSpec with generatesNotifs: true to make sure C++ has a corresponding MessageSpec.
- I am open to writing those message specs in Rust, but I am not sure what exactly do you mean. We can:
- implement entire functionality of parsing notification in Rust.
- implement it twice (in C++ with higher priority and in Rust in the meantime)
- implement entire functionality in C++ and some part of it in Rust (for future work)
- Implement some part in C++ and some part in Rust and use binary bridges
Which one did you actually had in mind?
Honestly it was just a thought... I haven't thought it through in much detail. But I think the thinking was mostly D (by necessity) – any potential Rust code would probably be called from the C++, at least initially.
Comment Actions
Use enum class for MessageTypes, add missing newlines to MessageSpec interface implementations, use camel case for message specs dictionary variable name.
Comment Actions
LGTM, I still think we should go lazy with the initialization but if I'm outvoted, then we can go with eager init. Accpeting.
Comment Actions
Definitely, I can create a task follow-up. I did not respond earlier since I thought it should be implemented prior to landing this diff so I started researching possibilities.
Comment Actions
I think we'll want some protection to make sure somebody doesn't create a MessageSpec in JS with generatesNotifs: true without also creating a MessageSpec in C++. One thing we could do is do a check at app boot for every JS MessageSpec with generatesNotifs: true to make sure C++ has a corresponding MessageSpec.
Sounds like a good idea
| native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageSpecs.h | ||
|---|---|---|
| 92 ↗ | (On Diff #12402) | I think we should avoid premature optimizations. In this case it doesn't sound too expensive to create a fixed number of objects at the start of the app, so keeping it eager makes the most sense to me. If we detect that this is a performance issue, we can always make it lazy. |
| 48–50 ↗ | (On Diff #12474) | We can probably simplify it |
| native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageSpecs/MultimediaMessageSpec.h | ||
| 10 ↗ | (On Diff #12474) | We usually use media as a name |
Comment Actions
Definitely, I can create a task follow-up. I did not respond earlier since I thought it should be implemented prior to landing this diff so I started researching possibilities.
Please link the task!! A link to a task is what I'm looking for here
Comment Actions
I created Linear issue with task to implement checking whether each MessageSpec in JS has corresponding MessageSpec in C++. https://linear.app/comm/issue/ENG-1159/ensure-that-all-messagespec-objects-in-libsharedmessagesmessage