[expo-notifications] Split compound emitter #7011
Conversation
| } else { | ||
| emitter.removeSubscription(subscription); | ||
| } | ||
| export function removeSubscription(subscription: Subscription) { |
There was a problem hiding this comment.
is there any difference between calling this and subscription.remove()? if not, this seems redundant.
There was a problem hiding this comment.
True, I agree! This is also what James pointed out in #6577 (comment)
removeTokenSubscriptionremovePushTokenSubscription
(although, is this function needed if developers can just callsubscription.remove()?)
, to which I responded with #6779 (comment)
I agree developers can just call
subscription.remove(), but eg.EventEmitter.tsand other event emitters (likeMediaLibrary) implement this method anyway.Maybe if we decide to remove this method from all event emitters it should be in a separate PR?
which has been approved of. Do you think we should remove this method now, or should we remove all of them immediately, or postpone the change until… some day?
There was a problem hiding this comment.
Gotcha. Sorry to make you justify this twice!! Let's leave it for now for consistency.
There was a problem hiding this comment.
This only proves that the review process is consistently good and thorough! 😃
5263e4c to
0cc6a0f
Compare
Why
Follow up to #6773 (comment).
How
Split
addNotificationListenerinto three:addNotificationReceivedListener,addNotificationsDroppedListener,addNotificationResponseReceivedListener.Test Plan
Added a test case to
test-suite, verifying thatnotificationReceivedListeneris called when a notification is received.