Details

Reviewers

tomek
atul
ashoat

Commits

rCOMM58b2bc7243cd: Start CommIOSNotifications implementation by adding JavaScript events handlers.

Summary

This differential starts CommIOSNotifications implementation by introducing notifications handlers that emit relevant JavaScript events and register them in NotificationCenter. All implementation here is copeied from Ashoats fork of RN notifications.

Test Plan

Build the app

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

marcin created this revision.Dec 28 2022, 4:41 AM

Herald added a subscriber: ashoat. · View Herald TranscriptDec 28 2022, 4:41 AM

marcin added a parent revision: D6065: Add NotificationsBridgeQueue module.Dec 28 2022, 4:44 AM

Harbormaster completed remote builds in B14823: Diff 20214.Dec 28 2022, 4:55 AM

marcin requested review of this revision.Dec 28 2022, 4:55 AM

marcin added a child revision: D6067: Implement native public notifications API.Dec 28 2022, 5:01 AM

Is there a more idiomatic way we can pass events, without directly using RCTBridge? See here for the old way. I can't see if TurboModule have a different mechanism for dispatching events, but might be worth reviewing that as well.

In D6066#181996, @ashoat wrote:

Is there a more idiomatic way we can pass events, without directly using RCTBridge? See here for the old way. I can't see if TurboModule have a different mechanism for dispatching events, but might be worth reviewing that as well.

I have just tested subclassing RCTEventEmitter and it worked correctly. It will even simplify some JS code in further diffs. I should update this diff stack shortly , so that CommIOSNotifications implements RCTEventEmitter.

Implement CommIOSNotifications as RCTEventEmitter

Harbormaster completed remote builds in B15018: Diff 20495.Jan 2 2023, 5:32 AM

Seems reasonable, but I'm really far from being an expert.

Question

native/ios/Comm/CommIOSNotifications/CommIOSNotifications.mm
41 ↗	(On Diff #20495)	If this gets called at the start of the application, then how do we already have `localNotification` set?

To your queue

Hmm, appears to be some bug in Phabricator where I can't Request Changes

This revision now requires changes to proceed.Jan 3 2023, 10:40 AM

marcin added inline comments.Jan 4 2023, 12:59 AM

native/ios/Comm/CommIOSNotifications/CommIOSNotifications.mm
41 ↗	(On Diff #20495)	This is explained here: https://developer.apple.com/documentation/uikit/uiapplicationlaunchoptionslocalnotificationkey. If the app was launched by the system as a reaction for local notification, it is present in `launchOptions`.

Observe NSNotificationCenter from the beggining, not when the first JavaScript listener is added.

Harbormaster completed remote builds in B15070: Diff 20561.Jan 4 2023, 1:59 AM

Thanks for explaining

This revision is now accepted and ready to land.Jan 4 2023, 5:45 PM

Remove unused constant

Harbormaster completed remote builds in B15177: Diff 20689.Jan 9 2023, 7:20 AM

Sorry, read through this a bit more critically and have a couple more comments...

native/ios/Comm/CommIOSNotifications/CommIOSNotifications.mm
24–27 ↗	(On Diff #20689)	Why not specify this on the `.h` file?
133 ↗	(On Diff #20689)	Reverse conditions to reduce indentation and to exit early
135 ↗	(On Diff #20689)	What's the point of this lazy initialization?

This revision now requires changes to proceed.Jan 9 2023, 9:56 AM

Consequently use dot syntax
Remove event handlers that are redundant since no JS code subscribes those events.
Implement notification parsing in Objective C, so that we don't have to do the parsing in JS and use exact and simple types for notifications.

ashoat accepted this revision.Jan 17 2023, 7:16 AM

This revision is now accepted and ready to land.Jan 17 2023, 7:16 AM

Harbormaster completed remote builds in B15405: Diff 21007.Jan 17 2023, 7:27 AM

ashoat requested changes to this revision.Jan 17 2023, 1:55 PM

ashoat added inline comments.