Skip to content
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

chore: deduplicate subsequent identify calls #1649

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/__tests__/posthog-core.identify.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,22 @@ describe('identify()', () => {
)
expect(instance.featureFlags.setAnonymousDistinctId).not.toHaveBeenCalled()
})

it('does not call $set when duplicate properties are passed', () => {
instance.identify('a-new-id', { email: '[email protected]' }, { howOftenAmISet: 'once!' })
instance.identify('a-new-id', { email: '[email protected]' }, { howOftenAmISet: 'once!' })

expect(beforeSendMock).toHaveBeenCalledTimes(1)
expect(instance.featureFlags.setAnonymousDistinctId).not.toHaveBeenCalled()
})

it('calls $set when different properties are passed with the same distinct_id', () => {
instance.identify('a-new-id', { email: '[email protected]' }, { howOftenAmISet: 'once!' })
instance.identify('a-new-id', { email: '[email protected]' }, { howOftenAmISet: 'twice!' })

expect(beforeSendMock).toHaveBeenCalledTimes(2)
expect(instance.featureFlags.setAnonymousDistinctId).not.toHaveBeenCalled()
})
})

describe('invalid id passed', () => {
Expand Down
23 changes: 20 additions & 3 deletions src/posthog-core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ import { PostHogExceptions } from './posthog-exceptions'
import { SiteApps } from './site-apps'
import { DeadClicksAutocapture, isDeadClicksEnabledForAutocapture } from './extensions/dead-clicks-autocapture'
import { includes, isDistinctIdStringLike } from './utils/string-utils'
import { getIdentifyHash } from './utils/identify-utils'

/*
SIMPLE STYLE GUIDE:
Expand Down Expand Up @@ -279,6 +280,7 @@ export class PostHog {
analyticsDefaultEndpoint: string
version = Config.LIB_VERSION
_initialPersonProfilesConfig: 'always' | 'never' | 'identified_only' | null
_cachedIdentify: string | null

SentryIntegration: typeof SentryIntegration
sentryIntegration: (options?: SentryIntegrationOptions) => ReturnType<typeof sentryIntegration>
Expand Down Expand Up @@ -306,7 +308,7 @@ export class PostHog {
this.analyticsDefaultEndpoint = '/e/'
this._initialPageviewCaptured = false
this._initialPersonProfilesConfig = null

this._cachedIdentify = null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was trying to decide if we should store this in local storage but i think i prefer this isn't cached outside of memory...

i'm not super familiar with identify and person processing so this feels like a safe step to take to test the waters

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah was also thinking about that, the current implementation helps in SPA setups, but hard page reloads would only be fixed by local storage...

this.featureFlags = new PostHogFeatureFlags(this)
this.toolbar = new Toolbar(this)
this.scrollManager = new ScrollManager(this)
Expand Down Expand Up @@ -1430,9 +1432,21 @@ export class PostHog {
// let the reload feature flag request know to send this previous distinct id
// for flag consistency
this.featureFlags.setAnonymousDistinctId(previous_distinct_id)

this._cachedIdentify = getIdentifyHash(new_distinct_id, userPropertiesToSet, userPropertiesToSetOnce)
} else if (userPropertiesToSet || userPropertiesToSetOnce) {
// If the distinct_id is not changing, but we have user properties to set, we can go for a $set event
this.setPersonProperties(userPropertiesToSet, userPropertiesToSetOnce)
// If the distinct_id is not changing, but we have user properties to set, we can check if they have changed
// and if so, send a $set event

if (
this._cachedIdentify !== getIdentifyHash(new_distinct_id, userPropertiesToSet, userPropertiesToSetOnce)
) {
this.setPersonProperties(userPropertiesToSet, userPropertiesToSetOnce)

this._cachedIdentify = getIdentifyHash(new_distinct_id, userPropertiesToSet, userPropertiesToSetOnce)
} else {
logger.info('A duplicate posthog.identify call was made with the same properties. It has been ignored.')
}
}

// Reload active feature flags if the user identity changes.
Expand Down Expand Up @@ -1464,6 +1478,8 @@ export class PostHog {
// Update current user properties
this.setPersonPropertiesForFlags(userPropertiesToSet || {})

// if exactly this $set call has been sent before, don't send it again - determine based on hash of properties

joshsny marked this conversation as resolved.
Show resolved Hide resolved
this.capture('$set', { $set: userPropertiesToSet || {}, $set_once: userPropertiesToSetOnce || {} })
}

Expand Down Expand Up @@ -1568,6 +1584,7 @@ export class PostHog {
this.surveys?.reset()
this.persistence?.set_property(USER_STATE, 'anonymous')
this.sessionManager?.resetSessionId()
this._cachedIdentify = null
if (this.config.__preview_experimental_cookieless_mode) {
this.register_once(
{
Expand Down
10 changes: 10 additions & 0 deletions src/utils/identify-utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { jsonStringify } from '../request'
import type { Properties } from '../types'

export function getIdentifyHash(
distinct_id: string,
userPropertiesToSet?: Properties,
userPropertiesToSetOnce?: Properties
): string {
return jsonStringify({ distinct_id, userPropertiesToSet, userPropertiesToSetOnce })
}
Loading