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(flags): store setOnce properties in locally persisted feature flag props #1716

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

havenbarnes
Copy link
Contributor

Changes

Context: https://posthoghelp.zendesk.com/agent/tickets/24489

Include setOnce properties in the locally persisted feature flag person properties: https://posthog.com/docs/feature-flags/adding-feature-flag-code#automatic-overrides

These are stored in local storage, but only used by feature flags for passing into the /decide call. This means that we should be safe from setOnce properties inadvertently overwriting previous property values.

Checklist

  • Tests for new code (see advice on the tests we use)
  • Accounted for the impact of any changes across different browsers
  • Accounted for backwards compatibility of any changes (no breaking changes in posthog-js!)

Copy link

vercel bot commented Feb 4, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
posthog-js ✅ Ready (Inspect) Visit Preview Feb 4, 2025 11:28pm

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR modifies the feature flag property persistence in posthog-js to include setOnce properties, ensuring they are stored locally and only used for /decide calls without affecting existing property values.

  • Modified identify and setPersonProperties methods in /src/posthog-core.ts to merge userPropertiesToSetOnce with userPropertiesToSet
  • Properties are only used for feature flag /decide calls, preventing unintended overwrites of existing values
  • Changes maintain backwards compatibility and work consistently across browsers
  • Functionality verified through existing feature flag and identify tests in /functional_tests/ and /playwright/

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@havenbarnes havenbarnes added bump minor Bump minor version when this PR gets merged bump patch Bump patch version when this PR gets merged and removed bump minor Bump minor version when this PR gets merged labels Feb 4, 2025
Copy link

github-actions bot commented Feb 4, 2025

Size Change: +312 B (+0.01%)

Total Size: 3.3 MB

Filename Size Change
dist/array.full.es5.js 268 kB +30 B (+0.01%)
dist/array.full.js 371 kB +30 B (+0.01%)
dist/array.full.no-external.js 369 kB +30 B (+0.01%)
dist/array.js 183 kB +34 B (+0.02%)
dist/array.no-external.js 181 kB +30 B (+0.02%)
dist/main.js 184 kB +34 B (+0.02%)
dist/module.full.js 371 kB +30 B (+0.01%)
dist/module.full.no-external.js 369 kB +30 B (+0.01%)
dist/module.js 183 kB +34 B (+0.02%)
dist/module.no-external.js 182 kB +30 B (+0.02%)
ℹ️ View Unchanged
Filename Size
dist/all-external-dependencies.js 215 kB
dist/customizations.full.js 13.8 kB
dist/dead-clicks-autocapture.js 14.5 kB
dist/exception-autocapture.js 9.48 kB
dist/external-scripts-loader.js 2.64 kB
dist/recorder-v2.js 115 kB
dist/recorder.js 115 kB
dist/surveys-preview.js 69.4 kB
dist/surveys.js 72.4 kB
dist/tracing-headers.js 1.76 kB
dist/web-vitals.js 10.4 kB

compressed-size-action

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.

1 participant