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): Add new debugging property $used_bootstrap_value, $feature_flag_bootstrapped_response and $feature_flag_bootstrapped_payload to $feature_flag_called event #1567

Merged
merged 15 commits into from
Dec 12, 2024

Conversation

havenbarnes
Copy link
Contributor

@havenbarnes havenbarnes commented Nov 28, 2024

Changes

Same change for bootstrapping as PostHog/posthog-js-lite#320

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

…response` and `$feature_flag_bootstrapped_payload` to `$feature_flag_called` event
Copy link

vercel bot commented Nov 28, 2024

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

Name Status Preview Updated (UTC)
posthog-js ✅ Ready (Inspect) Visit Preview Dec 12, 2024 7:06pm

Copy link

github-actions bot commented Nov 28, 2024

Size Change: +5.21 kB (+0.16%)

Total Size: 3.21 MB

Filename Size Change
dist/array.full.es5.js 261 kB +518 B (+0.2%)
dist/array.full.js 364 kB +521 B (+0.14%)
dist/array.full.no-external.js 363 kB +521 B (+0.14%)
dist/array.js 178 kB +521 B (+0.29%)
dist/array.no-external.js 177 kB +521 B (+0.3%)
dist/main.js 179 kB +521 B (+0.29%)
dist/module.full.js 364 kB +521 B (+0.14%)
dist/module.full.no-external.js 363 kB +521 B (+0.14%)
dist/module.js 178 kB +521 B (+0.29%)
dist/module.no-external.js 177 kB +521 B (+0.3%)
ℹ️ View Unchanged
Filename Size
dist/all-external-dependencies.js 206 kB
dist/customizations.full.js 12.6 kB
dist/dead-clicks-autocapture.js 14.4 kB
dist/exception-autocapture.js 9.48 kB
dist/external-scripts-loader.js 2.48 kB
dist/recorder-v2.js 115 kB
dist/recorder.js 115 kB
dist/surveys-preview.js 57.6 kB
dist/surveys.js 63.3 kB
dist/tracing-headers.js 1.75 kB
dist/web-vitals.js 10.4 kB

compressed-size-action

Copy link
Contributor

@dmarticus dmarticus left a comment

Choose a reason for hiding this comment

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

stamped with a comment about making sure this has parity for the other client-side SDKs.

src/posthog-featureflags.ts Show resolved Hide resolved
@havenbarnes havenbarnes added the bump patch Bump patch version when this PR gets merged label Dec 2, 2024
@havenbarnes havenbarnes requested a review from dmarticus December 4, 2024 20:31
Copy link
Contributor

@dmarticus dmarticus left a comment

Choose a reason for hiding this comment

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

:shipit:

@havenbarnes havenbarnes requested a review from dmarticus December 4, 2024 21:21
@havenbarnes havenbarnes added bump minor Bump minor version when this PR gets merged and removed bump patch Bump patch version when this PR gets merged labels Dec 4, 2024
@havenbarnes havenbarnes changed the title chore(flags): Add new debugging property $feature_flag_bootstrapped_response and $feature_flag_bootstrapped_payload to $feature_flag_called event chore(flags): Add new debugging property $used_bootstrap_value, $feature_flag_bootstrapped_response and $feature_flag_bootstrapped_payload to $feature_flag_called event Dec 4, 2024
@@ -32,6 +32,7 @@ describe('featureflags', () => {
get_property: (key) => instance.persistence.props[key],
capture: () => {},
decideEndpointWasHit: false,
receivedFlagValues: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this PR we are essentially find+replacing the existing flag decideEndpointWasHit for this new, more accurately named receivedFlagValues flag. The prior will now mean that the endpoint truly was hit with no errors, and the latter will mean that there are flag values available for the SDK to return.
decideEndpointWasHit is now really only used for passing back the $used_bootstrap_value flag in $feature_flag_called events.

@havenbarnes
Copy link
Contributor Author

@dmarticus added $used_bootstrap_value to this, see comment above for explanation of the big diff

Copy link
Contributor

@dmarticus dmarticus left a comment

Choose a reason for hiding this comment

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

third ship is the charm

Copy link
Contributor

@dmarticus dmarticus left a comment

Choose a reason for hiding this comment

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

third ship is the charm

@@ -47,7 +47,7 @@ If they seem to be failing unexpectedly, check grafana for ingestion lag at http
log(JSON.stringify(files, null, 2))

// the deadline is the same for each assert, as the ingestion lag will be happening in parallel
const deadline = Date.now() + 1000 * 60 * 30 // 30 minutes
const deadline = Date.now() + 1000 * 60 * 120 // 30 minutes
Copy link
Contributor

@dmarticus dmarticus Dec 9, 2024

Choose a reason for hiding this comment

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

pedantic, but you should change the comment to match the time (it's 120 minutes now).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hoping to not leave this around, but may do so depending on what others think

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah fair 'nuff

@havenbarnes havenbarnes merged commit cf9f91e into main Dec 12, 2024
13 checks passed
@havenbarnes havenbarnes deleted the bootstrapped-flag-evt-property branch December 12, 2024 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump minor Bump minor version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants