launchdarkly / react-native-client-sdk

LaunchDarkly Client-side SDK for React Native
Other
45 stars 32 forks source link

Shared anonymous context keys do not work on Android #247

Closed ngrewe closed 3 months ago

ngrewe commented 4 months ago

Describe the bug

The LD documentation indicates that it should be possible to share a key for anonymous contexts, but this is not currently working with the React Native SDK under Android and hasn't been for some time. Here the Android native module initialises the client configuration with generateAnonymousKeys(true) in a hardcoded fashion. This in turn makes the Android client SDK overwrite any key specified by the user.

This behaviour is wildly inconsistent with what the iOS SDK does. Under iOS the SDK generates a key (and marks the context as anonymous) only if the user did not supply one. It has also caused inflated MCI numbers for us. We don't need or want to target non-registered users of our app individually, so this is wasted spend for us. We would appreciate it if at the very least this behaviour were made configurable through the React Native SDK.

Thanks,

Niels

To reproduce

Initialise an anonymous user with a shared key on multiple Android devices:

const config: LDConfig = {
    mobileKey: 'key',
    stream: true,
    enableAutoEnvAttributes: true,
};
const ldClient = new LDClient();
await ldClient.configure(config, { kind: 'user', key: 'shared-key-for-anonymous-users', anonymous: true });

Expected behavior

The user specified key should be respected and there should only be one context created in LaunchDarkly.

Logs

-/-

SDK version

8.0.1

Language version, developer tools

not relevant

OS/platform

Android.

yusinto commented 4 months ago

This is fixed in v10 which is a complete re-write of the LaunchDarkly React Native SDK in pure TypeScript. As mentioned in v9's readme, please consider upgrading to v10 to solve this issue.

ngrewe commented 4 months ago

Thanks for the info. You have been shipping the buggy code for almost a year and it has caused us excess costs of about 20%, so I hope that you understand if we are not entirely satisfied with the suggestion of switching to a complete rewrite that was released just a few weeks ago. We would prefer a solution that does not put that onus on us…

Cheers,

Niels

louis-launchdarkly commented 4 months ago

Hello @ngrewe, thank you for reaching out and I apologize that this behavior is causing this problem for you. LaunchDarkly made this change when we released the Android 3.x SDK as we were trying to ensure data safety (to not having context that has no key), but missed this use case where a customer wants an anonymous Context while keeping the key.

We realized this is an issue and have not done this in other SDKs (e.g. iOS), and have heard much feedback for React Native that the difference between Android and iOS is a huge pain. Also, because the key override is a documented behavior of the Android SDK, some customers expect this behavior, even if it doesn't work for your use case (and LaunchDarkly also thinks it is not ideal). To avoid a lot of cascading major version changes, we are planning to fix this in Android SDK after React Native and Flutter SDK stop depending on the Android and iOS SDKs.

Because of the reasoning above, the team had been focusing on the RN SDK 10.x rewrite, written in pure TypeScript that has this (and many Android not equal iOS issues) fixed. We will look into fixing the Android SDK next, and figure out what we can do with the RN SDK 8.x and 9.x afterward. I understand this doesn't provide an immediate fix for you who are relying on the 8.x SDK, but hope this will at least give you some visibility on the progress of fixing this issue.

ngrewe commented 4 months ago

Hi @louis-launchdarkly! Thanks for the explanation. The key override may be ‘a documented behavior of the Android SDK’, but it's definitely not documented (or at least not discoverably so) for the RN SDK. We had to reverse engineer the cause after accidentally discovering erroneous contexts in the live events view. I totally understand not wanting to break backwards compatibility for other users, which is why I suggested allowing SDK users to opt out of this behaviour. Can you explain to me why something like that would be a problem?

louis-launchdarkly commented 4 months ago

I read the Android SDK's code and your suggestion will likely work, let me check with the team and see if is there anything that I am missing.

louis-launchdarkly commented 3 months ago

Looking at this history, there could have been an intention to avoid having Android or iOS-specific configuration in the configuration. However, given this causes the issue you are experiencing (and LaunchDarkly moved away from this wrapper-style SDK completely), adding an Android-specific configuration makes sense.

I have discussed this with the team, and we will add this fix to 7.x, 8.x, and 9.x so customers like you will not be forced to update to the 10.x SDK for this fix. We will post another update on this issue when we release the fix.

louis-launchdarkly commented 3 months ago

Hello @ngrewe - the fix is released in 7.2.0, 8.1.0, and 9.1.0 last week. This should resolve the issue for you going forward. Thank you for your patience and the suggestion for the fix.