-
Notifications
You must be signed in to change notification settings - Fork 142
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
💥 [RUM-7704] Remove anonymous user feature flag for v6 #3216
Conversation
7c36d46
to
c5d3f2a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v6 #3216 +/- ##
==========================================
+ Coverage 93.51% 93.52% +0.01%
==========================================
Files 286 286
Lines 7552 7552
Branches 1718 1717 -1
==========================================
+ Hits 7062 7063 +1
+ Misses 490 489 -1 ☔ View full report in Codecov by Sentry. |
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
815a996
to
73db602
Compare
73db602
to
8e6e650
Compare
@@ -592,6 +592,7 @@ describe('startSessionManager', () => { | |||
|
|||
describe('tracking consent', () => { | |||
it('expires the session when tracking consent is withdrawn', () => { | |||
spyOn(Math, 'random').and.callFake(() => 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 suggestion: add a comment to explain why you need this
@@ -126,7 +129,7 @@ export function startSessionStore<TrackingType extends string>( | |||
|
|||
function synchronizeSession(sessionState: SessionState) { | |||
if (isSessionInExpiredState(sessionState)) { | |||
sessionState = getExpiredSessionState(sessionState) | |||
sessionState = getExpiredSessionState(sessionState, !!configuration.trackAnonymousUser) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥜 nitpick: it could be simpler to pass the configuration
object to getExpiredState
9d99746
to
67de507
Compare
@@ -592,6 +592,7 @@ describe('startSessionManager', () => { | |||
|
|||
describe('tracking consent', () => { | |||
it('expires the session when tracking consent is withdrawn', () => { | |||
spyOn(Math, 'random').and.callFake(() => 0) // mock random for anonymous uuid generation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 suggestion: I don't think the anonymous id value is tested in this test. Maybe remove this line?
@@ -16,8 +17,8 @@ const DURATION = 123456 | |||
const PRODUCT_KEY = 'product' | |||
const FIRST_ID = 'first' | |||
const SECOND_ID = 'second' | |||
|
|||
const EXPIRED_SESSION: SessionState = { isExpired: '1' } | |||
const EXPIRED_SESSION: SessionState = { isExpired: '1', anonymousId: '0' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 suggestion:
const EXPIRED_SESSION: SessionState = { isExpired: '1', anonymousId: '0' } | |
const EXPIRED_SESSION: SessionState = { isExpired: '1', anonymousId: jasmine.any(String) as string } |
@@ -136,6 +142,7 @@ describe('session store', () => { | |||
|
|||
describe('initialize session', () => { | |||
it('when session not in store, should initialize a new session', () => { | |||
spyOn(Math, 'random').and.callFake(() => 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 suggestion: remove this line, use jasmine.any(String)
@@ -454,6 +461,7 @@ describe('session store', () => { | |||
|
|||
describe('reinitialize session', () => { | |||
it('when session not in store, should reinitialize the store', () => { | |||
spyOn(Math, 'random').and.callFake(() => 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 suggestion: remove this line, use jasmine.any(String)
Motivation
Remove the anonymous id feature flag to prepare for v6 release
Changes
Removed feature flags everywhere
Fixed unit and e2e tests of session storage to include anonymous id
Testing
I have gone over the contributing documentation.