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

fix(capture): Always update stored person props from $set #820

Merged
merged 1 commit into from
Oct 4, 2023

Conversation

Twixes
Copy link
Member

@Twixes Twixes commented Oct 3, 2023

Changes

@simfish85 brought to my attention that every posthog.people.set() call fires an event, and I realized this can't quite be optimized away just by using the $set property in posthog.capture()'d events. That's because in the latter case, stored person properties don't get updated, potentially messing up feature flags.

This PR adds the handling around stored person props that posthog-people had to posthog-core. Let me know if this makes sense, I've not been involved in posthog-js lately.

Checklist

@github-actions
Copy link

github-actions bot commented Oct 3, 2023

Size Change: +665 B (0%)

Total Size: 724 kB

Filename Size Change
dist/array.full.js 177 kB +167 B (0%)
dist/array.js 119 kB +166 B (0%)
dist/es.js 118 kB +166 B (0%)
dist/module.js 118 kB +166 B (0%)
ℹ️ View Unchanged
Filename Size
dist/recorder-v2.js 95 kB
dist/recorder.js 58.3 kB
dist/surveys.js 38.7 kB

compressed-size-action

Copy link
Collaborator

@neilkakkar neilkakkar left a comment

Choose a reason for hiding this comment

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

Makes sense!

Is your plan to aggregate lots of set calls into a capture call to reduce no. of times it sends an event?

@Twixes
Copy link
Member Author

Twixes commented Oct 4, 2023

I thought about it, but gonna leave it at this for now. Just so that as a customer you can use either way of $setting and be sure both will work.

@Twixes Twixes added the bump patch Bump patch version when this PR gets merged label Oct 4, 2023
@Twixes Twixes merged commit 5ef19f5 into master Oct 4, 2023
12 checks passed
@Twixes Twixes deleted the persist-properties-outside-people branch October 4, 2023 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump patch Bump patch version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants