Details

Reviewers

tomek
atul
ashoat

Commits

rCOMMb94dec10352a: Export minimal set of CommIOSNotifications RCT methods to use it in AppDelegate

Summary

In order to correctly use CommIOSNotifications in AppDelegate we must call comsumeBackgroundQueue in ios.js. Therefore this differential exports minimal set of react-native methods that must be called from JavaScript.

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, 6:27 AM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptDec 28 2022, 6:27 AM

marcin added a parent revision: D6067: Implement native public notifications API.Dec 28 2022, 6:30 AM

marcin added a child revision: D6071: Use CommIOSNotifications in AppDelegate and in JavaScript to request permissions.Dec 28 2022, 6:37 AM

Harbormaster completed remote builds in B14829: Diff 20220.Dec 28 2022, 6:41 AM

marcin requested review of this revision.Dec 28 2022, 6:41 AM

marcin added reviewers: tomek, atul.Dec 29 2022, 11:12 AM

Rebase

marcin edited child revisions, added: D6138: Introduce CommIOSNotifications class in JavaScript; removed: D6071: Use CommIOSNotifications in AppDelegate and in JavaScript to request permissions.Jan 2 2023, 5:31 AM

Harbormaster failed remote builds in B15019: Diff 20496!Jan 2 2023, 5:32 AM

Harbormaster completed remote builds in B15019: Diff 20496.Jan 2 2023, 7:08 AM

All methods in this differential are copied from Ashoat's fork https://github.com/Ashoat/react-native-notifications/blob/master/RNNotifications/RNNotifications.m. Subtle visual differences are results of applying formatter.

Copy-pasting code requires considering the license of what you're copying. Instead, can you look at the existing code as inspiration and implement something similar, but in your own style?

Copy pasted code will be refactored.

Refactor not to have direct copy pasted methods

Harbormaster completed remote builds in B15179: Diff 20691.Jan 9 2023, 7:24 AM

Fix race condition in checkPermissions

Harbormaster completed remote builds in B15206: Diff 20731.Jan 9 2023, 10:51 AM

tomek accepted this revision.Jan 11 2023, 3:54 AM

tomek added inline comments.

native/ios/Comm/CommIOSNotifications/CommIOSNotifications.mm
293–298	I don't think this ternary follows our rules https://www.notion.so/commapp/Use-ternary-conditionals-sparingly-f4ba44a10259403592a1d15440a9847e
308–309	It feels like a statement `jsReady = YES` quite obviously means that we're marking the js thread as ready, so I don't think the comment is really useful.
344–346	This is formatted differently that other functions - we usually don't add whitespaces around colons
348	This syntax is inconsistent with the other place in this file

This revision is now accepted and ready to land.Jan 11 2023, 3:54 AM

tomek added a reviewer: ashoat.Jan 11 2023, 3:55 AM

This revision now requires review to proceed.Jan 11 2023, 3:55 AM

Please address concerns before landing

native/ios/Comm/CommIOSNotifications/CommIOSNotifications.mm
293–298	Yeah let's avoid this
308–309	Agree
344–346	This makes me suspect that something is wrong with `clang-format` precommit hooks, which reinforces the concerns I laid out in D6063
348	I personally prefer the dot syntax. @marcin, remember that it's your responsibility to rewrite this code in your own style – that style should be consistent throughout. Using inconsistent style makes me suspect that you're still copy-pasting

This revision is now accepted and ready to land.Jan 11 2023, 8:43 AM

Consequently use dot syntax.
Refactor ugly ternary operator
Remove redundant code.

Nit

native/ios/Comm/CommIOSNotifications/CommIOSNotifications.mm
268–269 ↗	(On Diff #21009)	It would probably be cleaner if you defined this callback before calling `requestAuthorizationWithOptions:completionHandler:`... that way it wouldn't be so indented, I think