feat: add history api autocapture pageviews #1886
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Size Change: +16.4 kB (+0.45%) Total Size: 3.68 MB
ℹ️ View Unchanged
|
src/types.ts
Outdated
| /** | ||
| * Whether to capture History API events (pushState, replaceState, popstate) | ||
| */ | ||
| captureHistoryEvents?: boolean | ||
|
|
There was a problem hiding this comment.
do we need this on remote config?
are you adding this in team settings as well? that's super cool!
There was a problem hiding this comment.
I was actually going to ask what the pattern is here; I see most of the extensions are on the remote config, but I don't think we would actually need it there, but if it is a good thing, sure!
There was a problem hiding this comment.
so, you only need to add it to remote config if you want to control whether it is active remotely from team settings
we don't control pageview capture iirc from remote settings so i think safe not to add this here
There was a problem hiding this comment.
yep, confirmed we don't control page view capture remotely so i'd skip this
| const result = originalPushState.apply(this, args) | ||
| try { | ||
| if (isEnabled()) { | ||
| instance.capture('$pageview', { navigation_type: 'pushState' }) |
There was a problem hiding this comment.
Just thinking through some edge cases here.
What about something like e.g. metabase where every query run updates the state? Or sites that automatically update query params when you visit (I've seen this with google ad links, e.g. when I google shoes and click the first shopping link https://arterton.co.uk/products/acme-wingtip-balmoral-oxford-8091?variant=43255266050221&utm_medium=product_sync&utm_source=google&utm_content=sag_organic&utm_campaign=sag_organic&gad_source=1&gclid=EAIaIQobChMIxsXc4PvNjAMVlJJQBh0Dew1OEAQYASABEgJ18vD_BwE it did this (it added the country code as a query param)
Maybe this should only trigger when the actual pathname changes, rather than the query or hash?
Or maybe those users just shouldn't use this 🤷
There was a problem hiding this comment.
That is a really good point. I think most use cases would be for only the pathname, but on that same link, if I click on different sizes, it changes the query for the variant, which some people may also want to track.
We could add an option on the config like { captureHistoryChange: 'always' | 'pathname' | 'never'} instead of just a boolean but I am afraid this could grow...
Thoughts @ivanagas ?
There was a problem hiding this comment.
I think we should only trigger a pageview on path change if possible and keep it as a boolean.
This seems to be what people expect. For example, the Docusaurus plugin captured when the hash changed unintentionally and people complained so we fixed it. Hash or state changes are probably handled better by autocapture or custom events.
There was a problem hiding this comment.
Cool! I've updated both PRs to follow that line and only capture pageviews when the path changes.
lricoy
force-pushed
the
feat/add-history-api-autocapture-pageviews
branch
from
f180342 to
e0d50ec
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR adds automatic pageview tracking for History API navigation events in the PostHog JavaScript SDK.
- Added new
capture_history_eventsconfiguration option (default:false) to enable automatic pageview tracking for History API navigation - Created
HistoryAutocaptureclass that monitorspushState,replaceState, andpopstateevents to capture pageviews - Implemented pathname-based tracking that only triggers pageviews when the URL path changes (ignoring query string/hash changes)
- Added comprehensive test suite covering various navigation scenarios, edge cases, and proper integration with PageViewManager
4 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
src/types.ts
Outdated
| * | ||
| * @default false | ||
| */ | ||
| capture_history_events: boolean |
There was a problem hiding this comment.
How about adding another option to capture_pageview, e.g. capture_pageview: 'history-change?
This is what people might expect capture_pageview to do anyway
There was a problem hiding this comment.
Makes sense to me, there is no capture_pageview option for posthog-js-lite, so it would be just the way we would configure this here, right? Asking because I am trying to keep those two in sync as much as possible.
lricoy
force-pushed
the
feat/add-history-api-autocapture-pageviews
branch
from
f85c74a to
f1810ee
Compare
…autocapture-pageviews
|
@ivanagas @pauldambra, this looks good to be merged. Do you guys want to take another look? |
lricoy
force-pushed
the
feat/add-history-api-autocapture-pageviews
branch
from
b53e9d0 to
1fc878c
Compare
|
TWIMC: I am updating the playground samples and checking some docs references before merging this, it should be good to go tomorrow |
Changes
This is a follow-up of PostHog/posthog-js-lite#446, which ports the same functionality here. The tests and code are structured in a similar way but I've changed them in a attempt to make they fit the standard of this project a bit more.
In summary, if
{ capture_pageviews: 'history_change' }config is provided, we will send history events as page views, but only on pathname changes (no query string or hash change should trigger pageviews, even when using the back button).Checklist