-
Notifications
You must be signed in to change notification settings - Fork 38
Support for IAM in RN Messaging package #139
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
Conversation
packages/messaging/js/Messaging.js
Outdated
| // @"onShow", @"onDismiss", @"shouldShowMessage",@"urlLoaded" | ||
| const eventEmitter = new NativeEventEmitter(RCTAEPMessaging); | ||
| eventListenerShow = eventEmitter.addListener('onShow', (event) => { | ||
| console.log(">>>>> onShow"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove these console logs and use SDK debug logs instead?
| @@ -0,0 +1,54 @@ | |||
| /* | |||
| Copyright 2021 Adobe. All rights reserved. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2022
| @@ -0,0 +1,29 @@ | |||
| /* | |||
| Copyright 2021 Adobe. All rights reserved. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2022
|
@shtomar-adb can you add a private flag to messaging's package.json file? like ->
Then the (Lerna) publish script will ignore this private package. lerna/lerna#536 (comment)
|
…lementation of Messaging APIs
sbenedicadb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good with a few questions. let me know when you make the changes we talked about on our call today and i'll re-review
| NSLog(@">>>> shouldShowMessageWithMessage Message id: %@", messageObj.id); | ||
| [cachedMessages setObject:messageObj forKey:messageObj.id]; | ||
| [self emitEventWithName:@"shouldShowMessage" body:@{@"id":messageObj.id, @"autoTrack":messageObj.autoTrack ? @"true" : @"false"}]; | ||
| dispatch_semaphore_wait(semaphore, DISPATCH_TIME_FOREVER); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add a comment here in code explaining why we're using the semaphore
| } | ||
|
|
||
| RCT_EXPORT_BLOCKING_SYNCHRONOUS_METHOD(shouldShowMessage: (BOOL) shouldShowMessage) { | ||
| self->shouldShowMessage = shouldShowMessage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any particular reason why you're using -> notation here instead of .?
e.g. - self.shouldShowMessage = shouldShowMessage;
| - (NSArray<NSString *> *) supportedEvents { | ||
| return @[@"onShow", @"onDismiss", @"shouldShowMessage",@"urlLoaded"]; | ||
| } | ||
|
|
||
| - (void) startObserving { | ||
| hasListeners = true; | ||
| } | ||
|
|
||
| - (void) stopObserving { | ||
| hasListeners = false; | ||
| } | ||
|
|
||
| - (void) emitEventWithName: (NSString *) name body: (NSDictionary<NSString*, NSString*> *) dictionary { | ||
| if(hasListeners){ | ||
| [self sendEventWithName:name body:dictionary]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this section of functions to support the synchronous calls to and from javascript?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This functions are used to send events(along with event data) from native to JS event listeners.
The boolean hasListeners indicates that if an event Listener exists in the JS end. Then only send the event otherwise RN will give warning.
The events are used to notify the JS about the MessageDelegate functions called. Which further in JS are used to call corresponding Message Delegate functions registered in the JS.
packages/messaging/README.md
Outdated
| ``` | ||
|
|
||
| **Podfile setup** | ||
| The In app Message APIs dependes on the AEP Messaging 1.1.0. This version is not yet published to the Cocoapods but is available in the public [github repository](https://github.com/adobe/aepsdk-messaging-ios/tree/staging). Add the following pod dependency in your applications Podfile under the application target. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: dependes -> depends
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AEPMessaging, v1.1.0
packages/messaging/README.md
Outdated
| ## Messaging SDK API usage | ||
| Messaging SDK APIs must be called from the native Android/iOS project of React Native app. | ||
| ## Push Message API usage | ||
| Push Mesage related APIs in the Messaging SDK must be called from the native Android/iOS project of React Native app. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: Mesage > messaging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so maybe phrase like Push messaging APIs in the SDK must be called...?
packages/messaging/README.md
Outdated
|
|
||
| ## In App Messages API reference | ||
| ### refreshInAppMessages | ||
| Initiates a network call to retrieve remote In-App Message definitions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In-App Message probably doesn't need to be capitalized here
packages/messaging/README.md
Outdated
| ``` | ||
|
|
||
| ### setMessagingDelegate | ||
| Sets the UI Message delegate to listen the Message lifecycle events. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sets the MessagingDelegate in AEPCore.
… messaging Merge the changes from upstream.
packages/messaging/README.md
Outdated
| ``` | ||
|
|
||
| ### saveMessage | ||
| Call to the saveMessage leads to in-memory caching of the Message object. Cached Message object can be later use to show message. This function should be called from **shouldShowMessage** of MessagingDelegate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Natively caches the provided Message object.
packages/messaging/README.md
Outdated
| message.setAutoTrack(true); | ||
| ``` | ||
| ### clearMessage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering if maybe we should change the name of this method to just clear?
message.clearMessage(); sounds a little redundant
message.clear(); sounds better
🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does sound better.
packages/messaging/README.md
Outdated
| urlLoaded(url: string, message: Message): void; | ||
| }; | ||
| ``` | ||
| Object of type MessagingDelegate can be create as shown below: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: create > created
| anotherWorkflowStatus = "complete"; | ||
| currentMessage.show(); | ||
| } | ||
|
|
||
| function shouldShowMessage(message: Message): boolean { | ||
| if(someOtherWorkflowStatus == "inProgress") { | ||
| // store the current message for later use | ||
| Messaging.saveMessage(message); | ||
| currentMessage = message; | ||
| return false | ||
| } | ||
|
|
||
| return true | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we're using different names for the same variable in the example:
anotherWorkflowStatus and someOtherWorkflowStatus
cacheung
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Readme, Android initializing example
Update
import com.adobe.marketing.mobile.edge.identity;
to
import com.adobe.marketing.mobile.edge.identity.Identity;
Added the installation instructions for react-native-aepmessaging v1.0.0-beta.2 pre release.
… messaging Merged the remote branch.
…e functions in android sync with iOS.
…f sample app to match with main branch.
|
Hi guys. Is there any plan on merging / including this in RN library? |
sbenedicadb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a couple of small things that need to be updated. i'll merge those changes into the messaging branch.
| AEPMobileAssurance.class] completion:^{ | ||
| [AEPMobileCore lifecycleStart:@{@"contextDataKey": @"contextDataVal"}]; | ||
| AEPMobileOptimize.class] completion:^{ | ||
| [AEPMobileCore configureWithAppId:@"3149c49c3910/aaaac75639c6/launch-813b9d67d95e-development"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'll make sure to revert this back to your-app-ID
| @@ -0,0 +1,99 @@ | |||
| /* | |||
| Copyright 2021 Adobe. All rights reserved. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to change to 2022
| #import "RCTAEPMessaging.h" | ||
| @import AEPMessaging; | ||
| @import AEPCore; | ||
| @import AEPServices; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can remove this since it's imported by the header
-a couple small updates from the pr review
yangyansong-adbe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, only a small fix is needed.
| MobileCore.setApplication(this); | ||
| MobileCore.setLogLevel(LoggingMode.DEBUG); | ||
| try { | ||
| UserProfile.registerExtension(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to add those extension registrations back.
add some of the extensions back into android sample app
Description
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: