-
Notifications
You must be signed in to change notification settings - Fork 139
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Size Change: +3.46 kB (+0.11%) Total Size: 3.24 MB
ℹ️ View Unchanged
|
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.
brain dumped from a quick scan... i've not really worked with identify so haven't thought about whether we should 🙈
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.
was thinking about this...
i don't think it's breaking (even though it's technically breaking) because this is the behaviour that I think humans expect, so I don't think we need the hit on bundle size of adding config to opt in and slow rollout and all that jazz.
we should label as minor change though (that's what does CI here)
I didn't approve because I don't know why we shouldn't do it - which is too many double negatives - let's update docs on posthog.com and get @joethreepwood to put it in changelog
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.
Personally I think this makes total sense. I can't think of a reason why it wouldn't want to be this way, plus we have a warning logged for it so good from me
Often duplicate $identify calls are made due to the SDK being incorrectly setup in the app, or due to re-rendering of providers.
The current behaviour is to only deduplicate $identify calls with no properties, but most $identify calls include properties like email, so they are not deduplicated. This PR extends deduplication to calls with properties.
Note: this is my first contribution to js sdk so would appreciate a thorough review.
...
Checklist