Page MenuHomePhabricator

Implement message typoes validator and import it in root.react.js for side effect
ClosedPublic

Authored by marcin on Jun 6 2023, 3:12 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 5, 11:52 PM
Unknown Object (File)
Tue, Mar 5, 9:24 PM
Unknown Object (File)
Mon, Mar 4, 3:01 AM
Unknown Object (File)
Feb 23 2024, 3:27 PM
Unknown Object (File)
Feb 23 2024, 3:11 PM
Unknown Object (File)
Feb 23 2024, 2:56 PM
Unknown Object (File)
Feb 23 2024, 1:53 PM
Unknown Object (File)
Feb 23 2024, 11:21 AM
Subscribers

Details

Summary

This differential itroduces a file with custom code that checks if all message specs that might generate notifications are implemented in C++ as well. In the case of missing C++ message spec an dalert is displayed. Additionally this
feature required us to alter MessageSpec type a little bit so that generatesNotifs field is now optional.

Test Plan

Build the app and launch it. Ensure alert is not displayed. Go to message-types-validator.js file and comment the check for generatesNotifs in message spec. Now an alert with 5 message types should be displayed.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

marcin requested review of this revision.Jun 6 2023, 3:29 AM
tomek added inline comments.
native/types/message-types-validator.js
23–27 ↗(On Diff #27456)

As discussed offline, we could consider having a banner (displayed at the top of the app) instead telling if there are unimplemented specs. The benefits are:

  1. It will be constantly visible, so that it will be harder forget about adding specs before committing
  2. It will be less disruptive than having to constantly dismiss the alert while the specs aren't yet implemented
  3. It won't encourage devs to implement placeholder specs implementations just to get rid of the alert
This revision is now accepted and ready to land.Jun 6 2023, 7:14 AM
native/types/message-types-validator.js
23–27 ↗(On Diff #27456)
lib/shared/messages/message-spec.js
113

Should this have been modified to return PushType? It looks like all of the cases where we return undefined have been removed, but I may be missing something

lib/shared/messages/message-spec.js
113

Changing to PushType results in flow error. We removed all cases when generatesNotifs method invariantly returned undefined. Other generatesNotifs methods might return undefined in some cases depending on the particular notification content. For example:

Cannot assign Object.freeze(...) to reactionMessageSpec because undefined [1] is incompatible with literal union [2] in
type argument R [3] of the return value of property generatesNotifs. [incompatible-type-arg]

     shared/messages/reaction-message-spec.js
      213│     );
      214│
      215│     if (targetMessageInfo?.creatorID !== notifTargetUserID) {
 [1]  216│       return undefined;
      217│     }
      218│     return action === 'add_reaction' ? pushTypes.NOTIF : pushTypes.RESCIND;
      219│   },

     /private/tmp/flow/flowlib_2fd0d9f42c28b9ff_501/core.js
 [3] 1809│ declare class Promise<+R> {

     shared/messages/message-spec.js
 [2]  113│   ) => Promise<PushType>,
lib/shared/messages/message-spec.js
113

Thanks for explaining!