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: Store sessionId info in cookie with localstorage #770

Merged
merged 1 commit into from
Aug 10, 2023

Conversation

benjackwhite
Copy link
Collaborator

@benjackwhite benjackwhite commented Aug 9, 2023

Changes

Related to PostHog/posthog#15026 but with same-root domains

Currently we only store the distinctId in the cookie when using localStorage+cookie (the default). This allows us to tie cross-domain sessions together. We don't currently have the $sesid value stored though, which means Recordings are not linked.

This updates the code to allow multiple values to be stored in the cookie, now including the special $sesid value.

NOTE: One missing thing here is good handling of multiple active domain sessions. For example if I have a session on posthog.com and then one on app.posthog.com, they will have the same session_id. But if the session times out on posthog.com but not on app.posthog.com then there will be two sessions with a race to decide which reset occurs. We could perhaps mitigate this by periodically "refreshing" from the cookie store but this feels overkill for now.

Checklist

@benjackwhite benjackwhite added the bump patch Bump patch version when this PR gets merged label Aug 9, 2023
@benjackwhite benjackwhite marked this pull request as ready for review August 9, 2023 12:58
@benjackwhite benjackwhite requested review from a team and mariusandra August 9, 2023 12:58
@github-actions
Copy link

github-actions bot commented Aug 9, 2023

Size Change: +176 B (0%)

Total Size: 665 kB

Filename Size Change
dist/array.full.js 172 kB +44 B (0%)
dist/array.js 114 kB +44 B (0%)
dist/es.js 113 kB +44 B (0%)
dist/module.js 114 kB +44 B (0%)
ℹ️ View Unchanged
Filename Size
dist/recorder-v2.js 93.6 kB
dist/recorder.js 58.3 kB

compressed-size-action

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.

I'd bump minor but otherwise 👍

@benjackwhite
Copy link
Collaborator Author

I'd bump minor but otherwise 👍

I was thinking - nothing about the api changes. But i guess it is a new "feature"...

@benjackwhite benjackwhite added bump minor Bump minor version when this PR gets merged and removed bump patch Bump patch version when this PR gets merged labels Aug 9, 2023
@benjackwhite benjackwhite merged commit fd04266 into master Aug 10, 2023
15 checks passed
@benjackwhite benjackwhite deleted the feat/session-id-cookie-domain branch August 10, 2023 08:13
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