bug/analytics reset no longer clears anonoymousId #655
Conversation
|
silesky
force-pushed
the
bug/analytics-reset-no-longer-clears-anonoymousId
branch
2 times, most recently
from
1baeca8 to
03c4cbb
Compare
| void analytics.track('foo') | ||
| expect(getAnonId()).toBeTruthy() | ||
| analytics.reset() | ||
| expect(getAnonId()).toBeFalsy() |
There was a problem hiding this comment.
Since the race condition was that the track call would eventually set anonId again, I think checking that it is falsey after the track call completes would cover it (and fail without these changes)
There was a problem hiding this comment.
I actually moved the test to a different file, tested both failure and success condition!
silesky
force-pushed
the
bug/analytics-reset-no-longer-clears-anonoymousId
branch
from
c50b59b to
7e21da2
Compare
| @@ -0,0 +1,70 @@ | |||
| import cookie from 'js-cookie' | |||
There was a problem hiding this comment.
Refactored to make type safe
silesky
force-pushed
the
bug/analytics-reset-no-longer-clears-anonoymousId
branch
4 times, most recently
from
4e3a058 to
255c040
Compare
silesky
force-pushed
the
bug/analytics-reset-no-longer-clears-anonoymousId
branch
from
255c040 to
4507681
Compare
| @@ -322,6 +322,48 @@ describe('Event Factory', () => { | |||
| innerProp: '👻', | |||
| }) | |||
| }) | |||
|
|
|||
| describe.skip('anonymousId', () => { | |||
There was a problem hiding this comment.
These tests currently don't work? Are they regressions or existing issues?
There was a problem hiding this comment.
Not regressions, they are just existing edge cases / undefined behavior that I uncovered when working on this bug.
| @@ -205,6 +205,15 @@ export class EventFactory { | |||
| } | |||
|
|
|||
| public normalize(event: SegmentEvent): SegmentEvent { | |||
| const anonymousIdOverride = event.options?.anonymousId | |||
There was a problem hiding this comment.
maybe this is more readble instead of the whole if block?
event.options?.anonymousId && this.user.anonymousId(event.options.anonymousId)
as event.anonymousId = id seems to be a noop and happens regardless of this line of code.
There was a race condition where the segment "normalize" plugin would asyncronously instantiate / overwrite the anonymousId field in browser storage right before the http request, thus making it impossible to call analytics.reset() after a typical method call. (for example, in a .track call). Moved this to the EventFactory so at the very least this browser storage action happens syncronously.
The only behavioral change is that if someone mutates event.anonymousId in a plug-in, it's not going to also implicitly update the global user anonymousId when that event gets sent.
this seems... fine to me? I wish we could go a step further and force everyone to call "setAnonymousId" explicitly if they wanted to override the global anonymousId for all events.