fix: use persistence instead of localStorage on surveys #2355
+388
−98
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Size Change: +14.2 kB (+0.28%) Total Size: 5.11 MB
ℹ️ View Unchanged
|
|
@pauldambra requested a review from you as well since we were doing some discussions in the issue we don't have a lot of data stored. 2 are simple values like boolean/strings, other one is only for surveys in progress, so it only stored until the survey is sent or dismissed so even if the persistence is set to cookie only it would work well |
marandaneto
reviewed
Sep 24, 2025
marandaneto
reviewed
Sep 24, 2025
| */ | ||
| export const getFromPersistenceWithLocalStorageFallback = (key: string, posthog?: PostHog) => { | ||
| try { | ||
| if (posthog?.persistence && !posthog.persistence.isDisabled()) { |
There was a problem hiding this comment.
this check is duplicate across the 3 methods, i'd extract and reuse it
marandaneto
reviewed
Sep 24, 2025
| localStorage.setItem(key, value) | ||
| } | ||
| } catch (e) { | ||
| SURVEY_LOGGER.error('Error setting survey seen on persistence', e) |
There was a problem hiding this comment.
Suggested change
| SURVEY_LOGGER.error('Error setting survey seen on persistence', e) | |
| SURVEY_LOGGER.error('Error setting survey seen on persistence or localStorage', e) |
marandaneto
reviewed
Sep 24, 2025
| } | ||
| return localStorage.getItem(key) | ||
| } catch (e) { | ||
| SURVEY_LOGGER.error('Error getting property from persistence or localStorage', e) |
There was a problem hiding this comment.
those logs would be better if it was logged when an error happened within the persistence or localStorage happened so it'd be easier to debug issues.
right now we'd not know whats throwing, the persistence or localStorage
There was a problem hiding this comment.
separate try/catch blocks for all of them now
|
@lucasheriques we have discussed about this if the localStorage wasnt added intentionally, have you checked the commit history, PRs and comments if there was any reason to use the localStorage directly instead of the persistence? since the API already existed back then, i wonder if doing this would break things for some reason that we are not aware of |
Co-authored-by: Manoel Aranda Neto <[email protected]>
|
@marandaneto I went back in the history, and found the first PR that added local storage functionality to surveys looks like using persistence didn't come up back then it might have been because surveys used to live on another package here. and that package just used localStorage directly i found other prs after that (#933 #964 #1200) but all of them just used localStorage directly in that case, I do think we should go ahead with this, as it should work for most implementations. I also tested with a posthog instance set to memory only, and it worked as expected |
marandaneto
approved these changes
Sep 25, 2025
lucasheriques
force-pushed
the
fix/use-persistence-instead-of-localStorage-directly-on-surveys
branch
from
dde549b to
50e8d67
Compare
|
@marandaneto i actually tested more throughly, and posthog does not work on incognito well without localStorage access. as it causes the survey to show up consistently without it so instead I made this commit to warn about it, but still fall back to localStorage. but it's behind a try/catch, so it should also avoid the exceptions users are seeing but it's a logic change regardless. any thoughts on it before i merge? |
marandaneto
reviewed
Sep 26, 2025
| } | ||
| if (isPersistenceEnabledWithLocalStorage(this._instance)) { | ||
| logger.warn( | ||
| 'Persistence does not include localStorage, but surveys it to work properly. Please set persistence to include localStorage to avoid this warning, or set disable_surveys to true. Falling back to localStorage usage directly to maintain backwards compatibility.' |
There was a problem hiding this comment.
but surveys it to work properl
this sentence is confusing
marandaneto
reviewed
Sep 26, 2025
Comment on lines
+2
to
+5
| 'posthog-js': patch | ||
| --- | ||
|
|
||
| fix: use persistence instead of localStorage on surveys |
There was a problem hiding this comment.
i think the reason should be slightly better now with the possible odd behaviour after this change (also the PR description)
maybe it should not be a patch anymore?
marandaneto
reviewed
Sep 26, 2025
Comment on lines
+56
to
+58
| * We used to retrieve from localStorage directly. Now, we instead rely on the persistence API, since this is the preferred way to store data. | ||
| * But, since we might have customers that might be using surveys with persistence disabled, we need to fallback to localStorage | ||
| * to maintain backwards compatibility. |
There was a problem hiding this comment.
i think the persistence config should mention that surveys will fallback to localStorage if needed
a good moment to review/engage with https://github.com/PostHog/product-internal/pull/854 and talk to the customer support team on how to handle (and communicate) this change that might break customers |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Closes #1898
Changes
Surveys used to rely on
localStoragedirectly to manage some properties we use for our functionality (surveySeen, lastSeenSurvey, and inProgressSurvey).Now, instead, we use posthog.persistence, but maintain backwards compatibility by using localStorage if persistence is disabled. This is necessary as we can have customers that are using surveys, but might not be using persistence. Unlikely, but can happen
Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file