fix an issue that delete token does not trigger token refresh notification or delegate#5339
Conversation
charlotteliang requested review from ryanwilson, paulb777 and maksymmalyhin and removed request for ryanwilson
yeah, I left the public API header change out of this PR. |
I will kick out a separate PR for the integration test for this as we have configuration needs to be done. |
| print("Failed to get FID: ", error) | ||
| return | ||
| } | ||
| self.instanceID = fid ?? "None" |
There was a problem hiding this comment.
I think this should be nil if it doesn't exist, right? Otherwise consumers of this could think that an instanceID is available but the String is actually "None" which isn't valid.
There was a problem hiding this comment.
Done. Good catch!
| _ = NotificationCenter.default | ||
| .publisher(for: Notification.Name.MessagingRegistrationTokenRefreshed) | ||
| .map { $0.object as? String } | ||
| .receive(on: RunLoop.main) | ||
| .assign(to: \Identity.token, on: identity) |
There was a problem hiding this comment.
I believe you'll need to store these AnyCancellable values returned somewhere, likely in a Set<AnyCancellable> otherwise the subscription will only last the lifetime of this code block.
| // Subscribe to fid changes | ||
| _ = NotificationCenter.default | ||
| .publisher(for: Notification.Name.FIRInstallationIDDidChange) |
There was a problem hiding this comment.
Ooh - looks like we need to change the API name here as well (shouldn't have a FIR prefix).
| Installations.installations().installationID(completion: { fid, error in | ||
| if let error = error as NSError? { | ||
| print("Failed to get FID: ", error) | ||
| return | ||
| } | ||
| self.identity.instanceID = fid ?? "None" | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Does the notification not include information about the FID? If not we should look at adding it.
Fix#5338: When a token is deleted by calling
deleteTokenmethod, messaging doesn't trigger FIRMessagingRegistrationTokenRefreshedNotification notification or calling the FIRMessagingDelegatemessaging:didReceiveRegistrationToken:method.Hence the token parameter in the delegate API
messaging:didReceiveRegistrationToken:should be nullable, a change we might need to get in the next breaking change API.Update:
I also cleanup the code and realize IID send out two notifications (kFIRInstanceIDDefaultGCMTokenNotification, kFIRInstanceIDTokenRefreshNotification) when fcm token is updated. Sometimes it calls one, while other time it calls the other. These two notifications are essentially doing the exactly the same thing. So I replaced one that is not in the public API of IID.
Note: I was trying out combine with FIRMessagingRegistrationTokenRefreshedNotification and want to use token refresh notification/delegate to update the environment variable identity to reflect on UI on demand rather than manual update, then I discover this issue.