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

feat: Add __posthog_debug query string #1108

Merged
merged 2 commits into from
Apr 3, 2024
Merged

Conversation

robbie-c
Copy link
Member

@robbie-c robbie-c commented Apr 2, 2024

Changes

Add a query string to help with debugging the first visit to a site.

We already have the local storage version of this, but the query string method allows testing the order of operations when first visiting a site in a private browser tab, where local storage doesn't exist

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

@@ -124,7 +124,7 @@ export const defaultConfig = (): PostHogConfig => ({
save_referrer: true,
capture_pageview: true,
capture_pageleave: true, // We'll only capture pageleave events if capture_pageview is also true
debug: false,
debug: (location?.search && location.search.indexOf('__posthog_debug=true') !== -1) || false,
Copy link
Member Author

@robbie-c robbie-c Apr 2, 2024

Choose a reason for hiding this comment

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

Ugly because we have to support IE11, so no URLSearchParams or even string.includes

Copy link
Member

Choose a reason for hiding this comment

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

surprised we don't already have a convenience function for is blah in the URL but let's not overcomplicate by adding one if we don't already 😊

Copy link
Member

Choose a reason for hiding this comment

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

We do have _isString so you could really belt and braces by replacing location?.search with _isString(location?.search) but that's very nit picky and I don't think it adds enough safety to the check to care

Copy link

github-actions bot commented Apr 2, 2024

Size Change: +589 B (0%)

Total Size: 943 kB

Filename Size Change
dist/array.full.js 225 kB +151 B (0%)
dist/array.js 124 kB +146 B (0%)
dist/es.js 124 kB +146 B (0%)
dist/module.js 125 kB +146 B (0%)
ℹ️ View Unchanged
Filename Size
dist/exception-autocapture.js 12.1 kB
dist/recorder-v2.js 106 kB
dist/recorder.js 106 kB
dist/surveys-module-previews.js 62 kB
dist/surveys.js 58.3 kB

compressed-size-action

@robbie-c robbie-c marked this pull request as ready for review April 2, 2024 13:12
@robbie-c robbie-c added the bump minor Bump minor version when this PR gets merged label Apr 2, 2024
@robbie-c robbie-c changed the title Add __posthog_debug query string feat: Add __posthog_debug query string Apr 2, 2024
Copy link
Member

@pauldambra pauldambra left a comment

Choose a reason for hiding this comment

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

didn't run it so nitpicky comment worrying about type-safety but other than that this seems a great idea 🚀 🚢

@@ -124,7 +124,7 @@ export const defaultConfig = (): PostHogConfig => ({
save_referrer: true,
capture_pageview: true,
capture_pageleave: true, // We'll only capture pageleave events if capture_pageview is also true
debug: false,
debug: (location?.search && location.search.indexOf('__posthog_debug=true') !== -1) || false,
Copy link
Member

Choose a reason for hiding this comment

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

surprised we don't already have a convenience function for is blah in the URL but let's not overcomplicate by adding one if we don't already 😊

@@ -124,7 +124,7 @@ export const defaultConfig = (): PostHogConfig => ({
save_referrer: true,
capture_pageview: true,
capture_pageleave: true, // We'll only capture pageleave events if capture_pageview is also true
debug: false,
debug: (location?.search && location.search.indexOf('__posthog_debug=true') !== -1) || false,
Copy link
Member

Choose a reason for hiding this comment

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

We do have _isString so you could really belt and braces by replacing location?.search with _isString(location?.search) but that's very nit picky and I don't think it adds enough safety to the check to care

@robbie-c robbie-c merged commit 4bbf0cf into main Apr 3, 2024
13 checks passed
@robbie-c robbie-c deleted the feat/add-debug-query-string branch April 3, 2024 11:00
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