-
Notifications
You must be signed in to change notification settings - Fork 134
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: start session recording on url trigger #1451
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Hey @richard-better! 👋 |
Size Change: +17.1 kB (+0.6%) Total Size: 2.85 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks great 🚀
didn't run it yet but 💯
@@ -36,6 +44,7 @@ import { gzipSync, strFromU8, strToU8 } from 'fflate' | |||
|
|||
const BASE_ENDPOINT = '/s/' | |||
|
|||
const ONE_MINUTE = 1000 * 60 | |||
const FIVE_MINUTES = 1000 * 60 * 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very picky... and will have almost no impact individually but a little code golf helps the bundle size over time so you could have
const FIVE_MINUTES = 1000 * 60 * 5 | |
const FIVE_MINUTES = ONE_MINUTE * 5 |
i tend to dislike that style of writing and would rather fix the bigger problems in the bundle sizing but little things like that tend to help (although the bundler can behave surprisingly 🤣)
@@ -906,11 +964,19 @@ export class SessionRecording { | |||
this._pageViewFallBack() | |||
} | |||
|
|||
// Check if the URL matches any trigger patterns | |||
this._checkUrlTrigger() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could check this only when the URL changes (we have a hook in this class already)
or return early in checkUrlTrigger
if there's no reason to run the check
wandering towards early optimisation though - not blocking at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested locally and this works now
Changes
Client part of PostHog/posthog#25340
Checklist