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

fix: Use populated user when checking if user is anonymous #590

Merged
merged 1 commit into from
Oct 31, 2023

Conversation

laurawarr
Copy link
Contributor

In this PR there was a change that switched from using the populated user to using the initial user to check if the user is anonymous.

isAnonymous would only be true on the initial user if the SDK is explicitly called with this property. The populated user will have isAnonymous set to true if a user id is not specified.

This would cause the cached anonymous user ID to be cleared on every initialization, resulting in more MAUs than expected.

@laurawarr laurawarr requested review from ajwootto and a team October 27, 2023 22:05
@vercel
Copy link

vercel bot commented Oct 27, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
js-sdks-next-js-page-router ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 31, 2023 8:27pm
js-sdks-web-elements ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 31, 2023 8:27pm
js-sdks-with-provider ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 31, 2023 8:27pm

@@ -216,7 +216,7 @@ export class DevCycleClient<

this.eventEmitter.emitInitialized(true)

if (initialUser.isAnonymous) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i thought it was the intention that the user passed into the SDK needed to either have isAnonymous=true, or have a user id set, otherwise you'd receive an error? That's what the comments on the DevCycleUser interface say anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like that's an old comment that wasn't removed when this change was made. After anonymous user ID caching was implemented, if the user ID isn't passed then it's assumed the user is anonymous

this.isAnonymous = user.isAnonymous || !user.user_id

UofPx has been using the SDK with an empty user object, and as a result of this each initialization clears the ID cache and counts as an MAU

Copy link
Contributor

Choose a reason for hiding this comment

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

ok. Can you fix that comment and maybe add a test for this?

@laurawarr laurawarr force-pushed the fix-anon-user-cache branch 2 times, most recently from 4c54ae5 to 6181617 Compare October 30, 2023 15:21
@laurawarr laurawarr requested a review from ajwootto October 30, 2023 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants