braze-inc / braze-react-native-sdk

Public repo for the Braze React Native SDK
https://www.braze.com
Other
64 stars 84 forks source link

[Bug]: Breaking change not specified in release notes #200

Closed Minishlink closed 1 year ago

Minishlink commented 1 year ago

Which Platforms?

iOS

Which React Native Version?

0.70

Which @braze/react-native-sdk SDK version?

2.0.0

Repro Rate

100%

Description

braze.inAppMessagePresenter = [[BrazeInAppMessageUI alloc] init]; needs to be specified now after Braze *braze = [BrazeReactBridge initBraze:configuration];. Otherwise in app messages aren't shown. Before v2, it was not needed. It should be added to the release notes breaking changes.

hokstuff commented 1 year ago

Hi @Minishlink,

We will add a note to the breaking Changelog entry of the 2.0.0 release to explicitly call out the integration steps noted here. The official Swift SDK documentation is in the final stages prior to release, so the hope is that the migration docs for all our cross-platform SDKs will be more straightforward once that is publicly available.

Currently, the braze.inAppMessagePresenter is set in subscribeToInAppMessage(), so if the host app calls it (e.g. in our example code), the field will be set already. However, it might instead be preferable to be explicitly set in the AppDelegate as an integration step, since associating the Braze SDK with a UI isn't directly correlated to subscribing to in-app message data. Let us know if you have further thoughts!

One thing to note is that the delegate of the inAppMessageUI contains this method which is used to send the in-app message from the iOS layer to the Javascript layer. I believe this can be set via the bridge variable here though

Minishlink commented 1 year ago

Hello, yes the API would be clearer with subscribeToInAppMessage separated from the Braze UI part. And I'm not sure a JS function might make sense for the UI part since there is no Android equivalent (at least by default). IMHO, the braze UI for in app messages should be the default for easier integration, and you would disable it if you want to show your own UI. You might be interested in what other similar SDKs also do with in app messages : they typically have a "do not disturb mode" that can be enabled/disabled from JS, so that in app messages are queued while the app is showing a splashscreen/loading initial data etc.

hokstuff commented 1 year ago

Hi @Minishlink,

We have released SDK version 4.0.0 which adds a Breaking Changelog entry to 2.0.0 and also makes the requested changes to automatically add the default Braze UI without needing to call subscribeToInAppMessage().

Thanks!

jarredt commented 1 year ago

Hi -- after upgrading from 1.32.0 to 5.0.0, I still had to add this code manually to my AppDelegate.swift to make in-app messages work:

let inAppMessageUI = BrazeInAppMessageUI()
braze.inAppMessagePresenter = inAppMessageUI

I noticed that the change in 4.0.0 moved the default logic from subscribeToInAppMessages to the startObserving method. But doesn't startObserving only get executed if/when a listener is actually added (ref)? Our JS code doesn't subscribe to any Braze listeners currently, so I think that may be why this isn't getting called for us...

Should this instead be moved to the initBraze method?

jerielng commented 1 year ago

Hey @jarredt, the default in-app message presenter is assigned in startObserving since it's designed to be used in conjunction with the listener event 'inAppMessageReceived' by registering with Braze.addListener.

There are three paths here that you can take based on your use case:

  1. If you wish to use the default IAM UI as-is with the default IAM presentation logic, you should first call Braze.addListener('inAppMessageReceived') and then call subscribe to in-app messages with useBrazeUI set to true
  2. If you wish to listen for incoming in-app messages but want to use your own IAM UI, you can first call Braze.addListener('inAppMessageReceived') and then call subscribe to in-app messages with useBrazeUI set to false. Then create and assign your own IAM presenter to the braze instance at the native layer.
  3. For any other cases, you can call neither of these methods and instead define your own presenter manually as you mentioned in your case and assign an IAM presenter delegate as appropriate.

If your use case doesn't fit this setup, please let us know if there's an alternatives you feel should be supported. Thank you!

jarredt commented 1 year ago

Thanks @jerielng! Case 1 or 3 will work for me. I'll be doing 3 because it seems odd to set up a listener at the JS level, that I won't be subscribing to, just to enable the messages to appear using the default UI.

I would still advocate for considering moving this logic out of the listener setup and into the SDK init logic directly. My reading of this issue's conversation about decoupling the UI presentation from the listener/subscription behavior, and the language of the 4.0.0 CHANGELOG, has me thinking that may have been the original intent of the change?

Here's the CHANGELOG entry:

The iOS bridge now automatically attaches the default In-App Message UI with the braze instance, without needing to call subscribeToInAppMessage(). This updates the behavior from 2.0.0 to simplify integration.

Sounds like that's not quite right -- it's only automatically attached if you call Braze.addListener('inAppMessageReceived') and Braze.subscribeToInAppMessages with useBrazeUI set to true. That doesn't seem to really solve the decoupling issue -- and it doesn't appear to be documented, unless I missed it? The Braze RN integration docs for in-app messages say:

Native in-app messages display automatically on Android and iOS when using React Native. This article covers customizing and logging analytics for your in-app messages for apps using React Native.

If the logic will be retained in the listener setup, would be great to have the requirement to set the listener in JS documented. Thanks for your attention to this!