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

make react no longer supply a default UUID key #159

Closed
wants to merge 1 commit into from
Closed

Conversation

brooswit
Copy link
Contributor

No description provided.

@@ -9,7 +8,7 @@ interface AllFlagsLDClient {

const initLDClient = async (
clientSideID: string,
user: LDUser = { key: uuid() },
user: LDUser = {},
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with doing this is that a user object with no key at all is invalid (unless it has anonymous:true, which case you get the "make a UUID or use a cached UUID" behavior). The SDK will refuse to initialize with such a user. Therefore, this would be a breaking change for anyone who omits the user parameter when they call initLDClient.

I think we probably shouldn't have allowed the user parameter to be omitted in the first place (it is not optional in the JS SDK or any of the other SDKs), but if we're to be compatible with the existing API, we need to have a valid user object. If you change this to { key: '' }, I believe the SDK will not complain— however, we should test manually to verify this; also, I don't know if events will be counted on the dashboard in that case, so we would need to test that too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the insight @eli-darkly.

I may not have time to test this out until next week. If someone else wants to take this over that would be okay with me. Otherwise, I'll comment here when I have time to work on this more.

@brooswit brooswit closed this Jun 24, 2019
@brooswit brooswit deleted the jw/ch41783 branch June 24, 2019 20:39
LaunchDarklyCI pushed a commit that referenced this pull request Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants