launchdarkly / android-client-sdk

LaunchDarkly Client-side SDK for Android
Other
45 stars 23 forks source link

Android SDK not at parity with iOS SDK #186

Closed johnnywang closed 1 year ago

johnnywang commented 2 years ago

Is this a support request? No

Describe the bug This is more a bug that comes as a result of using the React Native SDK, but manifests as a result of differences in implementation between the Android and iOS SDKs, so it seems like this is a better place to raise the issue. If this does belong in the RN SDK issues instead let me know and I can re-open there instead.

As surfaced in https://github.com/launchdarkly/react-native-client-sdk/issues/121, we discovered that LD's mobile SDKs don't officially support hot reload. However, we had discovered a workaround at the time whereby we could always close the client first, then call configure again, which would bypass the "already initialized" issue and allow us to (seemingly) use the SDK as one would expect, even in cases of hot reloads.

However, it turned out that this only worked for iOS, and not Android. A bit of digging around into the issue showed that:

I suspect this is due to differences in how the two native SDKs are implemented, rather than how the RN SDK is implemented.

To reproduce

  1. Look at the repro in https://github.com/launchdarkly/react-native-client-sdk/issues/121 (which surfaces when you do a hot reload)
  2. Now, instead of using the snippet in step 4 there, do the following and see how initialized returns false now in iOS (after a hot reload) as a result of the preliminary client.close():
    const App: () => Node = () => {
    let client = new LDClient();
    client.close().isInitialized().then((isInitialized) => {
    console.log('initialized? ', isInitialized);
    if (!isInitialized) {
      client
        .configure(
          {mobileKey: '<insert_key>'},
          {key: 'user_key'},
        )
        .then(() => console.log('success'));
    }
    });
  3. Now observe how this does not work in Android

Expected behavior While this is an undocumented/unofficial workaround/use case, I'd expect the two SDKs to behave similarly. But parity aside, it seems wrong that closing the client doesn't reset/reject the initialization state correctly in Android

SDK version

  "dependencies": {
    "launchdarkly-react-native-client-sdk": "^6.1.0",
    "react": "17.0.2",
    "react-native": "0.67.3"
  },

Language version, developer tools React Native, Flipper, yarn

OS/platform MacOS 12.1

Additional context I've previously reached out to customer support around this, and was told there were not plans to bring the two SDKs to parity, and that they'd put in a feature request. I didn't see anything in Github so opening it up here for tracking.

louis-launchdarkly commented 2 years ago

Hello @johnnywang, sorry for updating this response - we are still discussing this internally.

louis-launchdarkly commented 2 years ago

We filed part of the fix for isInitialized() for RN internally as 167623.

louis-launchdarkly commented 1 year ago

Hello @johnnywang, we just released https://github.com/launchdarkly/react-native-client-sdk/releases/tag/6.2.3 in React Native SDK, which should bring the isInitialized() behavior between iOS and Android closer when using the RN SDK. Please let us know does the fix work for your use case. Feel free to create another Support request or Github issue if there is anything else or a follow-up to this.

louis-launchdarkly commented 1 year ago

The is an issue with the RN 6.2.3 fix interacting with Android. We reverted the change so I reopened this to keep track of it.

eli-darkly commented 1 year ago

Just in terms of the Android SDK itself (since that's where this issue was opened), I would say that I'm not sure we should be considering any SDK's implementation of the "am I initialized" method to be a norm that other SDKs should be standardized to in the case where the SDK has already been closed. The definition of the close() method is that it's what you do when you are completely done with the LDClient that you've been using, aren't going to use that client instance any more, and want to release its resources. The client object after that point should be considered to be in an invalid/undefined state. If you reinitialize the SDK after that point, you are receiving a different instance of LDClient.

johnnywang commented 1 year ago

That's a fair point, but for context, this (admittedly undocumented) pathway is the only way to get LD to work in a RN app that supports hot reload: without it, LD just simply cannot be used

eli-darkly commented 1 year ago

@johnnywang Again, I can't speak to the details of the RN implementation as it currently is, and I'm not saying we shouldn't change anything. But I do think that if we're going to change something, it would be best to simply fix the RN implementation, rather than "fixing" either the iOS or the Android SDK to turn an undocumented and meaningless behavior into a standard.

eli-darkly commented 1 year ago

Anyway, we are continuing to discuss it internally.

louis-launchdarkly commented 1 year ago

Circle back to this, we have rewritten and improved the RN SDK (7.x - latest https://github.com/launchdarkly/react-native-client-sdk/releases/tag/7.1.4). Please open a new issue on RN SDK if this is still an issue.