Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: disable direct channels API on iOS, fixes #3674 #3733

Merged
merged 4 commits into from
Jul 14, 2020

Conversation

MrLoh
Copy link
Contributor

@MrLoh MrLoh commented Jun 2, 2020

Description

As discussed in #3674 Direct Channel API on iOS was deprecated, so the SDK shouldn't set shouldEstablishDirectChannel = YES anymore. I tested that this doesn't affect the delivery of notifications on iOS.

Related issues

Fixes #3674

Release Summary

Remove support for deprecated iOS Direct Channel API (#3674)

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
    • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan

Tested that notifications (including those with data payloads) still arrive on iOS in all 3 states (killed, background, running)

@CLAassistant
Copy link

CLAassistant commented Jun 2, 2020

CLA assistant check
All committers have signed the CLA.

@MrLoh
Copy link
Contributor Author

MrLoh commented Jun 2, 2020

There is a comment related to shouldEstablishDirectChannel here:

// Receive data messages on iOS 10+ directly from FCM (bypassing APNs) when the app is in the foreground.
// To enable direct data messages, you can set [Messaging messaging].shouldEstablishDirectChannel to YES.
- (void)messaging:(nonnull FIRMessaging *)messaging didReceiveMessage:(nonnull FIRMessagingRemoteMessage *)remoteMessage {

I think it can just be removed, but maybe @Salakar or @Ehesp who have the git blame for this can advice.

Salakar added 2 commits June 3, 2020 12:43
This test will no longer function on simulators as a direct message channel is no longer established.
@Salakar Salakar changed the title Disable direct channels API on iOS, fixes #3674 feat: disable direct channels API on iOS, fixes #3674 Jun 3, 2020
@mikehardy mikehardy requested review from Ehesp and Salakar July 11, 2020 07:24
@mikehardy
Copy link
Collaborator

@Salakar / @Ehesp the related link has the discussion but the change is nearly trivial, if you take as given that direct channel is going away upstream (it is), then setting this as the default is the thing to do

@Salakar Salakar merged commit 8c9f4f5 into invertase:master Jul 14, 2020
hmhm2292 pushed a commit to hmhm2292/react-native-firebase that referenced this pull request Jul 13, 2021
…tase#3733)

* set shouldEstablishDirectChannel to NO

* tests: make `onMessage` test Android only

This test will no longer function on simulators as a direct message channel is no longer established.

Co-authored-by: Mike Diarmid <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants