-
Notifications
You must be signed in to change notification settings - Fork 35
add handleJavascriptMessage in AEPMessaging #519
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
add handleJavascriptMessage in AEPMessaging #519
Conversation
const ENV_ID_Messaging = "3149c49c3910/4f6b2fbf2986/launch-7d78a5fd1de3-development"; | ||
MobileCore.initializeWithAppId(ENV_ID_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.
This will be reverted before merging to staging
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.
sure, could you update this line.
...id/src/main/java/com/adobe/marketing/mobile/reactnative/messaging/RCTAEPMessagingModule.java
Show resolved
Hide resolved
```html | ||
<html> | ||
<head> | ||
<script type="text/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.
function callNative(action) {
try {
// Android
if (
window.myInappCallback &&
typeof window.myInappCallback.run === 'function'
) {
window.myInappCallback.run(action);
}
// iOS
else if (
window.webkit &&
window.webkit.messageHandlers &&
window.webkit.messageHandlers.myInappCallback &&
typeof window.webkit.messageHandlers.myInappCallback.postMessage === 'function'
) {
window.webkit.messageHandlers.myInappCallback.postMessage(action);
} else {
console.log('No native bridge available');
}
} catch (e) {
console.log('Error calling native handler:', e);
}
}
can replace with this
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.
@namArora3112 this is part of the custom HTML I have created for testing, and was intended for internal testing only.
The snippet added in the docs is the same that is present in the already existing tutorials for Native code - https://developer.adobe.com/client-sdks/edge/adobe-journey-optimizer/in-app-message/tutorials/native-from-javascript/#html
If we are deviating from the standard examples shared in the rest of SDK Documentation, can you share some documentation around best practices on creating custom HTML so that we can provide appropriate examples in our tutorials.
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.
Have added minor comments. can merge it after addressing that
onDismiss: msg => console.log('dismissed!', msg), | ||
onShow: msg => console.log('show', msg), | ||
onShow: msg => { | ||
console.log('show', msg); |
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.
are we going to use console log functions directly
or there a global log method available for logging purpose?
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 dont have a logging utility available, we use the console log functions directly in the test app.
onShow: msg => console.log('show', msg), | ||
onShow: msg => { | ||
console.log('show', msg); | ||
msg.handleJavascriptMessage('myInappCallback', (content: string) => { |
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.
Nit: would recommend to use constants from a file
* @param {function} handler: The method or closure to be called with the body of the message created in the Message's JavaScript | ||
*/ | ||
handleJavascriptMessage(handlerName: string, handler: (content: string) => void) { | ||
// cache the callback |
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 add checks for the handler
for example something like this
if (!handlerName) throw new Error("handlerName required"); if (typeof handler !== "function") throw new Error("handler must be a function");
@ReactMethod | ||
public void handleJavascriptMessage(final String messageId, final String handlerName) { | ||
Presentable<?> presentable = presentableCache.get(messageId); | ||
if (presentable == null || !(presentable.getPresentation() instanceof InAppMessage)) return; |
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.
If the presentable is not found this just silently returns.
Could you please add logs or could we emit a warning why a handler wasn’t called.
} | ||
|
||
message.handleJavascriptMessage(handlerName) { [weak self] content in | ||
self?.emitNativeEvent( |
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 add consistency across platform for emitting events ?
in android method name is emitEvent and ios has emitNativeEvent
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.
the emitEvent and emitNativeEvent functions are existing helper methods from before. Lets keep this PR focused on handleJavascriptMessage related changes. Refactoring can be taken as a separate task if needed.
|
||
In the `onShow` function of `MessagingDelegate`, call `handleJavascriptMessage(handlerName: string, handler: (content: string) => void)` to register your handler. | ||
|
||
The name of the message you intend to pass from the JavaScript side should be specified in the first parameter. |
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 seems a little incomplete,
I guess, the handlerName is a unique identifier used both in your React Native app and your in-app message HTML.
Ideally, It must match exactly on both sides for the bridge call to work.
correct me if i am wrong?
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.
The name of the message you intend to pass from the JavaScript side should be specified in the first parameter.
This line intends to highlight that the name passed from Javascript side should be sent as the first parameter here. Please let me know if its not clear.
Followed this tutorial for reference: https://developer.adobe.com/client-sdks/edge/adobe-journey-optimizer/in-app-message/tutorials/native-from-javascript/
// Record - {messageId : {handlerName : callback}} | ||
const jsMessageHandlers: Record<string, Record<string, (content: string) => void>> = {}; | ||
const handleJSMessageEventEmitter = new NativeEventEmitter(RCTAEPMessaging); | ||
|
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 create a type for this callback as this is being used in multiple places
type JsMessageHandler = (content: string) => void;
|
||
// Registery to store inAppMessage callbacks for each message in Message.handleJavascriptMessage | ||
// Record - {messageId : {handlerName : callback}} | ||
const jsMessageHandlers: Record<string, Record<string, (content: string) => void>> = {}; |
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.
Every time we show a new in-app message, hanndleJavascriptMessage()
adds another entry to the jsMessageHandlers object. But when the message is dismissed, the screen is unmounted, or the message is never shown again, those handlers stay in memory instead of being removed.
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.
added clearJavascriptMessageHandlers method for clearing the memory.
@namArora3112 please have another look
expect(spy).toHaveBeenCalledWith(id); | ||
}); | ||
|
||
it('handleJavascriptMessage is called', async () => { |
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.
The unit tests seems a little incomplete.
can we add a test to verify that when a message is not found (due to a bad handlerName or expired cache), the sdk gracefully handles the error without emitting incorrect events or crashing.
78a650f
into
adobe:feat/messagingEnhancements-7.2.0
* PropositionItem class added along with unified track functionality (#507) * proposition item classes, track funcitonality etc * proposition item uuid added * json proposition item removed * removed unused guide * added htmlProposition , jsonPropositon item class * removed unused apis of content card * generatePropositionInteractionXdm removed * ios implementation of track functionality * uuid vs proposition map added * MessagingProposition class fixes * get proposition for surfaces android fixes * ios get proposition for surface fixes * getproposition for surface updates * MessagingView commented code removed * removed proposition cache with parent * clear cache in update proposition * message util clean up * proposition Item tracking usage refactored * activityID extraction function reusable * white space fixes * added read me for track api usage * removed unnecessary checks before calling track api for android native * refactored extra null checks before calling track api at ios native side * Addeed ios and android logs for null cases * added comments and logs for android native code * marked the trackContentCardInteraction and trackContentCardDisplay as deprecated * added tests for propositionItem.track * read me updated for proposition Item track usage --------- Co-authored-by: Naman Arora <namarora+adobe@adobe.com> * add handleJavascriptMessage in AEPMessaging (#519) * Messaging - handle javascript message * add js event listener and ios * typecaste Message object * address PR comments * refactor log statements * address PR comments * revert env file id changes * clear js handler when onDismiss is called * messaging version bump for 7.2.0 release (#526) * typecaste to Message object (#527) * read me updates regarding deprecated apis --------- Co-authored-by: namArora3112 <namarora@adobe.com> Co-authored-by: Naman Arora <namarora+adobe@adobe.com>
Description
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: