Can you also updates lib/types/messages.js? EDIT ah I see these are in the next diff

In general I personally find it better to introduce types along with code that actually uses them, but many people on the team do what you did here and introduce them in a separate diff. It just makes it much harder to test... eg. your Test Plan doesn't actually test any of the types at all, which means it's quite a poor test plan.

Separately, I think it might be good to add @ginsu to your list of reviewers for this work, given how recently he touched reactions

In D6961#207350, @ashoat wrote:

Can you also updates lib/types/messages.js? EDIT ah I see these are in the next diff

In general I personally find it better to introduce types along with code that actually uses them, but many people on the team do what you did here and introduce them in a separate diff. It just makes it much harder to test... eg. your Test Plan doesn't actually test any of the types at all, which means it's quite a poor test plan.

Sure, I didn't know whether I should divide it or not. Next time I will do it in one diff.

Can you clarify in what scenarios the id can be null? Usually we only see that for types that also have localID

In D6961#211322, @ashoat wrote:

Can you clarify in what scenarios the id can be null? Usually we only see that for types that also have localID

You are right. I couldn't find such scenario. I also tested the app and it still works as expected. Updating the diff.