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(flags): Remove feature flag payloads from persistence #1846

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

haacked
Copy link
Contributor

@haacked haacked commented Mar 21, 2025

We already store feature flag details in persistence, so we can calculate payloads from that rather than storing payloads twice. We still store active feature flags as well as calculated flags in persistence and that's probably just fine as the data is a lot less and we use those everywhere.

Changes

This is a refactoring. No behavioral changes other than $feature_flag_payloads is no longer stored in local storage.

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!)

Testing

  • Tested manually in an incognito browser to see that $feature_flag_payloads no longer is stored in local storage.

We already store feature flag details in persistence, so we can calculate payloads from that rather than storing both.
Copy link

vercel bot commented Mar 21, 2025

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

Name Status Preview Updated (UTC)
posthog-js ✅ Ready (Inspect) Visit Preview Mar 21, 2025 11:10pm

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 refactors feature flag payload handling in PostHog's JavaScript SDK by removing redundant storage of payloads in local storage, since they can be derived from existing feature flag details.

  • Added createFeatureFlagDetailsFromLegacyDecideResponse in /src/posthog-featureflags.ts to handle legacy response formats
  • Modified getFlagPayloads() to calculate payloads from flag details instead of reading from storage
  • Made id field optional in FeatureFlagMetadata type in /src/types.ts to support cases where ID isn't available
  • Updated tests in /src/__tests__/featureflags.test.ts to verify payload handling through flag details metadata
  • Added empty flags object to decideResponse mock in /playwright/utils/setup.ts for testing

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

@@ -1838,6 +1936,7 @@ describe('parseFeatureFlagDecideResponse', () => {
payload: 300,
Copy link
Contributor

Choose a reason for hiding this comment

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

syntax: Duplicate 'payload' property in metadata object

Suggested change
payload: 300,

Copy link

Size Change: +1.79 kB (+0.05%)

Total Size: 3.63 MB

Filename Size Change
dist/array.full.es5.js 301 kB +205 B (+0.07%)
dist/array.full.js 381 kB +180 B (+0.05%)
dist/array.full.no-external.js 380 kB +180 B (+0.05%)
dist/array.js 189 kB +174 B (+0.09%)
dist/array.no-external.js 187 kB +174 B (+0.09%)
dist/main.js 189 kB +174 B (+0.09%)
dist/module.full.js 381 kB +180 B (+0.05%)
dist/module.full.no-external.js 380 kB +180 B (+0.05%)
dist/module.js 189 kB +174 B (+0.09%)
dist/module.no-external.js 187 kB +174 B (+0.09%)
ℹ️ View Unchanged
Filename Size
dist/all-external-dependencies.js 220 kB
dist/customizations.full.js 14.1 kB
dist/dead-clicks-autocapture.js 14.5 kB
dist/exception-autocapture.js 9.94 kB
dist/external-scripts-loader.js 2.75 kB
dist/posthog-recorder.js 211 kB
dist/recorder-v2.js 115 kB
dist/recorder.js 115 kB
dist/surveys-preview.js 71.3 kB
dist/surveys.js 76.6 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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant