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: start session recording on url trigger #1451

Merged
merged 14 commits into from
Oct 17, 2024
Merged

Conversation

richard-better
Copy link
Contributor

@richard-better richard-better commented Oct 3, 2024

Changes

Client part of PostHog/posthog#25340

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

Copy link

vercel bot commented Oct 3, 2024

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

Name Status Preview Updated (UTC)
posthog-js ✅ Ready (Inspect) Visit Preview Oct 17, 2024 9:25am

@posthog-bot
Copy link
Collaborator

Hey @richard-better! 👋
This pull request seems to contain no description. Please add useful context, rationale, and/or any other information that will help make sense of this change now and in the distant Mars-based future.

Copy link

github-actions bot commented Oct 3, 2024

Size Change: +17.1 kB (+0.6%)

Total Size: 2.85 MB

Filename Size Change
dist/array.full.js 357 kB +1.9 kB (+0.53%)
dist/array.full.no-external.js 356 kB +1.9 kB (+0.54%)
dist/array.js 171 kB +1.89 kB (+1.12%)
dist/array.no-external.js 170 kB +1.89 kB (+1.13%)
dist/main.js 172 kB +1.89 kB (+1.11%)
dist/module.full.js 357 kB +1.9 kB (+0.53%)
dist/module.full.no-external.js 356 kB +1.9 kB (+0.54%)
dist/module.js 171 kB +1.89 kB (+1.12%)
dist/module.no-external.js 170 kB +1.89 kB (+1.13%)
ℹ️ View Unchanged
Filename Size
dist/all-external-dependencies.js 192 kB
dist/exception-autocapture.js 11.4 kB
dist/external-scripts-loader.js 2.35 kB
dist/recorder-v2.js 111 kB
dist/recorder.js 111 kB
dist/surveys-preview.js 59.8 kB
dist/surveys.js 66 kB
dist/tracing-headers.js 8.36 kB
dist/web-vitals.js 10.3 kB

compressed-size-action

@richard-better richard-better requested a review from a team October 7, 2024 08:00
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.

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
Copy link
Member

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

Suggested change
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 🤣)

src/extensions/replay/sessionrecording.ts Outdated Show resolved Hide resolved
src/extensions/replay/sessionrecording.ts Show resolved Hide resolved
src/extensions/replay/sessionrecording.ts Outdated Show resolved Hide resolved
src/extensions/replay/sessionrecording.ts Show resolved Hide resolved
src/extensions/replay/sessionrecording.ts Show resolved Hide resolved
src/extensions/replay/sessionrecording.ts Show resolved Hide resolved
@@ -906,11 +964,19 @@ export class SessionRecording {
this._pageViewFallBack()
}

// Check if the URL matches any trigger patterns
this._checkUrlTrigger()
Copy link
Member

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

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.

tested locally and this works now

@richard-better richard-better marked this pull request as ready for review October 17, 2024 09:36
@richard-better richard-better added the bump minor Bump minor version when this PR gets merged label Oct 17, 2024
@richard-better richard-better merged commit b72703f into main Oct 17, 2024
19 checks passed
@richard-better richard-better deleted the record-on-url-trigger branch October 17, 2024 09:49
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.

3 participants