Upgrade react native 0.79.4 + Removed Mixpanel from Native Android/IOS SDKs #106
Conversation
…w tracking - Replaced Mixpanel tracking calls in SCLoans with internal analytics events - Removed Mixpanel SDK dependency from gradle - Verified build and functionality on working version
dporwal-shipit
changed the title
|
React-Native Bridge for Android successfully propagating events from Native Android SDK(Producer) to Smart Investing Appln (Consumer): For SCGateway: For SCLoans: React-Native Bridge for IOS successfully propagating events from Native Android IOS(Producer) to Smart Investing Appln (Consumer): For SCGateway: For SCLoans: |
…intainability - Simplified event flow between native and JS layers - Improved code readability and reduced complexity - Easier to extend with new analytics events in future
…ved maintainability - Simplified event flow between native and JS layers - Improved code readability and reduced complexity - Easier to extend with new analytics events in future
There was a problem hiding this comment.
install the "prettier" vscode extention for formatting js files. make sure that it matches the older style
(here the diff should only show changes that you've already made)
| return SmallcaseGatewayNative.archiveSmallcase(safeIscid); | ||
| }; | ||
|
|
||
| /** | ||
| * Returns the native android/ios and react-native sdk version | ||
| * (internal-tracking) | ||
| * @returns {Promise} |
There was a problem hiding this comment.
why did we remove these return and param types?
|
|
||
| /** | ||
| * 🔕 Unsubscribe from Gateway Event | ||
| * @param {object} subscription - Subscription returned from startGatewayEventListening |
There was a problem hiding this comment.
these param and return types can be better. make it closer to the actual types, instead of generic types
| this.isInitialized = true; | ||
| console.log('[SCLoansEvents] Initialized for', Platform.OS); | ||
| } else { | ||
| console.warn('[SCLoansEvents] Native module not found for', Platform.OS); |
There was a problem hiding this comment.
this probably should be thrown as an error. shouldn't silently fail
| } | ||
|
|
||
| try { | ||
| const subscription = this.eventEmitter.addListener('scloans_notification', (eventData) => { |
There was a problem hiding this comment.
keep all these strings ("scloans_notification") as constants. don't use them directly
| type: eventData.eventType || eventData.type || 'unknown_event', | ||
| eventType: eventData.eventType || eventData.type || 'unknown_event', | ||
| data: eventData.data || eventData, | ||
| timestamp: eventData.timestamp || Date.now(), |
There was a problem hiding this comment.
do these fallbacks make sense?
please check with the analytics team on what should be the correct fallback values such that invalid values don't make it into real analysis
|
|
||
| this.subscriptions.push(subscription); | ||
|
|
||
| console.log('SCLoansEvents Subscribed to gateway events'); |
There was a problem hiding this comment.
remove all these console logs from the SDK
| const index = this.subscriptions.indexOf(subscription); | ||
| if (index > -1) { | ||
| this.subscriptions.splice(index, 1); | ||
| } |
| USER_IDENTIFY: 'scgateway_user_identify', | ||
| }; | ||
|
|
||
| export class SCGatewayEvents { |
There was a problem hiding this comment.
here again, a lot of the event emitter code is duplicated.
structure it in a way where things can be shared as much as possible
There was a problem hiding this comment.
Actually we are treating both the sdks as separate and isolated. So, created separate files to support this. Same is true in android implementation as well for Notification Center.
No description provided.