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: Change default storage to localStorage+cookie, store client session props #906

Closed
wants to merge 3 commits into from

Conversation

robbie-c
Copy link
Member

@robbie-c robbie-c commented Nov 21, 2023

Changes

Didn't need to change much as the localStorage+cookie already handles the migration! Just needed to change the default, sanity check that tests work, and add my own key to the list of cookie-persisted keys.

Supercedes: #875
Related to: https://github.com/PostHog/product-internal/pull/531 and #876

Before we merge, let's merge this first PostHog/posthog.com#7095 and make sure that nothing catches fire

  • merged it

Checklist

Copy link

github-actions bot commented Nov 21, 2023

Size Change: +777 B (0%)

Total Size: 746 kB

Filename Size Change
dist/array.full.js 176 kB +195 B (0%)
dist/array.js 118 kB +194 B (0%)
dist/es.js 118 kB +194 B (0%)
dist/module.js 118 kB +194 B (0%)
ℹ️ View Unchanged
Filename Size
dist/exception-autocapture.js 12 kB
dist/recorder-v2.js 104 kB
dist/recorder.js 58.4 kB
dist/surveys.js 41.5 kB

compressed-size-action

@robbie-c robbie-c changed the title Change default storage to localStorage+cookie, store client session props feat(web-analytics): Change default storage to localStorage+cookie, store client session props Nov 21, 2023
@robbie-c robbie-c marked this pull request as ready for review November 22, 2023 09:11
@robbie-c robbie-c added the bump minor Bump minor version when this PR gets merged label Nov 22, 2023
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.

Looks good to me. Pulled and ran locally and it migrated me to localStorage+cookie

Copy link
Collaborator

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

Wat. This already supports migrating? I had no idea :D

@posthog-bot
Copy link
Collaborator

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@robbie-c robbie-c removed the stale label Nov 30, 2023
@benjackwhite benjackwhite changed the title feat(web-analytics): Change default storage to localStorage+cookie, store client session props feat: Change default storage to localStorage+cookie, store client session props Dec 7, 2023
Copy link
Collaborator

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

I'm going to pull out the changed default stuff only and not the client_session_props.

Reason being - we might need to reconsider the approach given what we are learning about the cookie size and content potentially triggering things like Cloudflares security challenges

@posthog-bot
Copy link
Collaborator

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Collaborator

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

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 stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants