Skip to content
This repository was archived by the owner on Aug 30, 2023. It is now read-only.

feat: Abstract sessions to a class #91

Merged
billyvg merged 6 commits intomainfrom
feat/abstract-session-class
Jun 24, 2022
Merged

feat: Abstract sessions to a class #91
billyvg merged 6 commits intomainfrom
feat/abstract-session-class

Conversation

@billyvg
Copy link
Copy Markdown
Member

@billyvg billyvg commented Jun 21, 2022

This would provide a better interface for the session. This fixes/simplifies updating the session directly where previously it would not save to storage, only the session "instance" (object).

billyvg added 4 commits June 21, 2022 18:52
This would provide a better interface for the session. We can use setters to save to storage when we update the session.
@billyvg billyvg force-pushed the feat/abstract-session-class branch from 36b4e18 to 7af23d0 Compare June 21, 2022 22:52
@billyvg billyvg changed the title feat/abstract session class feat: Abstract sessions to a class Jun 21, 2022
Comment on lines +12 to +15
return new Session(
JSON.parse(window.sessionStorage.getItem(REPLAY_SESSION_KEY)),
{ stickySession: true }
);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If there's a saved item, we can assume stickySession is true.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If this was localStorage instead of sessionStorage then the assumption might not hold as well. If someone is doing a deploy and changing the config for example, they might expect that the saved data gets wiped instead.
With sessionStorage I'm not worried about that because the data doesn't live nearly as long.

@billyvg billyvg marked this pull request as ready for review June 21, 2022 22:56
@billyvg
Copy link
Copy Markdown
Member Author

billyvg commented Jun 21, 2022

This fixes the bug where sequenceId was not being updated. This seems like the most obvious way to update the session, so introduced a Session class and use getters to determine if we should update storage.

@billyvg billyvg requested a review from a team June 21, 2022 22:58
import { uuid4 } from '@sentry/utils';
import { saveSession } from './saveSession';

export interface SessionObject {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

exported but I can't see any imports. Related to the toJSON method lower in this file though.

Comment on lines +12 to +15
return new Session(
JSON.parse(window.sessionStorage.getItem(REPLAY_SESSION_KEY)),
{ stickySession: true }
);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If this was localStorage instead of sessionStorage then the assumption might not hold as well. If someone is doing a deploy and changing the config for example, they might expect that the saved data gets wiped instead.
With sessionStorage I'm not worried about that because the data doesn't live nearly as long.

billyvg and others added 2 commits June 22, 2022 14:26
Co-authored-by: Ryan Albrecht <ryan@ryanalbrecht.ca>
@billyvg billyvg merged commit 8efa31e into main Jun 24, 2022
@billyvg billyvg deleted the feat/abstract-session-class branch June 24, 2022 00:34
mydea pushed a commit to getsentry/sentry-javascript that referenced this pull request Nov 23, 2022
This would provide a better interface for the session. This fixes/simplifies updating the session directly where previously it would not save to storage, only the session "instance" (object).
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants