feat: Person Processing 3: Handle $initial props across sessions, send $initial props with all person processing events, remove __preview #1127
Conversation
robbie-c
changed the title
|
Size Change: +632 B (0%) Total Size: 950 kB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
didn't run it... but this looks good to me
coupling between persistence and sessionPersistence stands out for me across these changes - not a review of this more typing it so it goes in my brain as something to care about 🤣
I wonder if we should follow up with some cypress tests here to test the e2e behaviour of this new config 🤔
| @@ -112,12 +133,59 @@ describe('person processing', () => { | |||
| const identifyCall = onCapture.mock.calls[0] | |||
| expect(identifyCall[0]).toEqual('$identify') | |||
| expect(identifyCall[1].$set_once).toEqual({ | |||
| $initial_referrer: '$direct', | |||
| $initial_referring_domain: '$direct', | |||
| $initial_referrer: 'https://referrer.com', | |||
There was a problem hiding this comment.
I'll lazily assume that tests elsewhere cover $direct being the default
| if (this.config.store_google) { | ||
| this.sessionPersistence.update_campaign_params() | ||
| this.persistence.set_initial_campaign_params() | ||
| } | ||
| if (this.config.save_referrer) { | ||
| this.sessionPersistence.update_referrer_info() | ||
| this.persistence.set_initial_referrer_info() | ||
| } |
There was a problem hiding this comment.
is the coupling "hard" here... i.e. if you call this.sessionPersistence.update_campaign_params() should you always call this.persistence.set_initial_campaign_params() or vice versa?
we could make that explicit so that callers don't have to remember... these two persistence stores by necessity will be super coupled anyway
but I'm thinking out loud - feel free to ignore
There was a problem hiding this comment.
It's more than we wanted the initial params to be stored at a person level, as they are person-initial, whereas the other params need to be session level, so we get the correct session-initial props in the sessions table. I can add a comment
There was a problem hiding this comment.
To actually answer your question which I kinda avoided :D
While the caller is "us", I'm not too worried that we have to remember which persistence to us. The alternative would be to have a combined persistence, where the caller just says what it want to save and it calls the right storage. That would be doable but I'd worry about increasing the bundle size by doing so.
Changes
alwaysmode, not just inidentified_onlymode__preview_prefix on the config optionChecklist