feat: customising and configuring segment #1
Conversation
| @@ -0,0 +1,120 @@ | |||
| import { DestinationPlugin } from '../plugin'; | |||
There was a problem hiding this comment.
What is the plan to test this file? I don't see automated tests anywhere for it?
There was a problem hiding this comment.
Currently I do not plan to add test cases as they are out of scope for this ticket. We can create another ticket that takes care of all the test cases.
There was a problem hiding this comment.
Creating a ticket is a good option. Besides creating a ticket, Matt shared an idea with me recently on creating skeleton test functions with // TODO inserted inside of those failing tests.
You decide how you prefer to document this. Whatever method works best for you.
| @@ -538,6 +538,24 @@ export class SegmentClient { | |||
| await this.process(event); | |||
| this.logger.info('TRACK event saved', event); | |||
| } | |||
| async registerDevice(attributes: JsonMap) { | |||
There was a problem hiding this comment.
I was expecting this logic of register/delete device being implemented as a segment sdk plugin instead of a modification to the code. At first, I like that idea because that would perform less modification to the fork source code. But, open to ideas if this is needed.
There was a problem hiding this comment.
The logic inside of this function looks to me like it's only creating a tracked event on the segment SDK.
Instead of adding these 2 functions to the segment code, would we do this from the CIO RN SDK code?
class CustomerIO
registerDevice() {
segmentClient.trackEvent("Device Created or Updated")
}
}Would this also work? I would prefer not having to add code to the Segmet SDK if we dont have to.
There was a problem hiding this comment.
Agreed with Levi and Shahroz here, I think we should add these method to CIO RN SDK instead of adding to fork.
| const context = this.store.context.get() | ||
| const updatedContext = Object.assign({}, context?.device, attributes) | ||
| this.store.context.set({ device: updatedContext }) |
There was a problem hiding this comment.
Looking over this code, it looks like it exists so our SDK can keep track of the device in the SDK's storage.
Assuming this is true, I would prefer if we stored the registered device inside of the CIO SDK code instead of Segment code. Is there a reason that we have to do it in the segment code?
| @@ -538,6 +538,24 @@ export class SegmentClient { | |||
| await this.process(event); | |||
| this.logger.info('TRACK event saved', event); | |||
| } | |||
| async registerDevice(attributes: JsonMap) { | |||
There was a problem hiding this comment.
We need to add this in our package rather than Segments
| @@ -142,7 +142,7 @@ export type Config = { | |||
| maxBatchSize?: number; | |||
| trackDeepLinks?: boolean; | |||
| defaultSettings?: SegmentAPISettings; | |||
| autoAddSegmentDestination?: boolean; | |||
| autoAddCustomerIODestination?: boolean; | |||
There was a problem hiding this comment.
I think we didn't plan to expose types directly from segment in our initial proposal, keeping this in mind, I think we don't need to refactor this and can add autoAddCustomerIODestination in our config object if needed.
| @@ -538,6 +538,24 @@ export class SegmentClient { | |||
| await this.process(event); | |||
| this.logger.info('TRACK event saved', event); | |||
| } | |||
| async registerDevice(attributes: JsonMap) { | |||
There was a problem hiding this comment.
Agreed with Levi and Shahroz here, I think we should add these method to CIO RN SDK instead of adding to fork.
| const MAX_PAYLOAD_SIZE_IN_KB = 500; | ||
| export const CUSTOMERIO_DESTINATION_KEY = 'Customer.io Data Pipelines'; | ||
|
|
||
| export class CustomerioDestination extends DestinationPlugin { |
There was a problem hiding this comment.
Can't we utilize SegmentDestination here directly? Something like:
export class CustomerioDestination extends SegmentDestinationThere was a problem hiding this comment.
I agree that we can extend SegmentDestination directly here but it does not help with the the main property that's changed which is SEGMENT_DESTINATION_KEY. The property key of the class SegmentDestination can be changed that's consuming the global property SEGMENT_DESTINATION_KEY but the other instance where SEGMENT_DESTINATION_KEY is being used will continue using it. Does this make sense? We can hop on a quick call to talk in detail if that helps.
| this.add({ plugin: segmentDestination }); | ||
| if (this.config.autoAddCustomerIODestination === true) { | ||
| const customerioDestination = new CustomerioDestination(); | ||
| this.add({ plugin: customerioDestination }); | ||
| } |
There was a problem hiding this comment.
Just an idea. When setting up analytics object in our RN SDK, we can set autoAddSegmentDestination to false and add our destination like this:
analytics.add({ plugin: new CustomerioDestination() });This can help us minimize the changes required in fork.
There was a problem hiding this comment.
Refers: [CDP] RN iOS: Fork & Integrate Segment React Native SDK #11653
closes: [CDP] RN iOS: Update CIO SDK to utilize Segment#11654
PR Includes :
customerio-reactnativeCustomerIOdedicated pluginanalyticsobject