launchdarkly / ios-client-sdk

LaunchDarkly Client-side SDK for iOS (Swift and Obj-C)
https://docs.launchdarkly.com/sdk/client-side/ios
Other
68 stars 84 forks source link

notify observers about changes rooting from an `identify` call #256

Closed diederich closed 4 months ago

diederich commented 2 years ago

When setting the user via identify, any observers were not notified about flag changes, although the underlying store changed.

This PR fixes that by calling into the "notify machinery" when swapping out the underlying data.

Requirements

diederich commented 2 years ago

👋 Hey there - we just realised that calling identify did not call the feature-flag changed observers. I'm not sure this is by design or was an oversight. If this is by design - is there any other way to be notified of changes?

lukeredpath commented 2 years ago

I’ve just run into this problem - we have an observer that runs for the duration of an app session and an account switch feature that triggers a call to identify - the observer does not detect changes when calling identify. The observer is still working though and will pick up a change if I toggle a flag on and off.

We are using streaming mode - I wonder if this problem also affects polling mode.

lukeredpath commented 2 years ago

For anyone interested in a workaround while this PR is open, I have taken advantage of the fact that the client is taken offline temporarily whenever you call .identify - this means you can create a connection state observer and whenever the state changes to polling or streaming to indicate it is back online again, you can notify your observers of the current flag value.

jasdev commented 2 years ago

@gwhelanLD, @louis-launchdarkly — sorry for the @, but any chance we could get some eyes on this when y’all get a moment? 🙏🏽

louis-launchdarkly commented 2 years ago

Hello @diederich, @lukeredpath, @jasdev Apologize and thank you for reaching out, for looking at this, and for waiting. @gwhelanLD and I had discussed this, and here are a few thoughts:

1) The flagChangeNotifier.notifyObservers() method is currently used when the flag is changed from a Sync (i.e. someone modified the flag in LaunchDarkly, and the SDK received the change, which is different from this proposal, when the flag values are different, due to change in User context. So just calling notifyObservers may create confusion here.

2) However, it is reasonable to want to know "Did identify change the current flag values in the SDK?". Similarly, some users may not want to know as there is an identify event already to note that flag context is being replaced by identify.

3) To serve the diverging needs, it seems that it is helpful to have means to control this behavior. We will need to evaluate more before we can propose a solution, but we are looking into it.

Filed internally as 128729

fischman-bcny commented 2 years ago

@louis-launchdarkly any update on this / internal 128729 ?

brianmichel commented 9 months ago

Hey @louis-launchdarkly any chance we can get this merged? We'd love to delete our internal fork and use the main LD library, happy to help out here in any way, just let me know :D

louis-launchdarkly commented 9 months ago

Hello @brianmichel, I am really sorry for the late reply. We are currently doing an audit of all the Client-side SDKs from LaunchDarkly, and are trying to line up the behavior of each SDK with this request in mind. That will inform can we merge this or adopt the change in the new callback design.

patriknyblad commented 9 months ago

Just wanted to chime in here and say that we also have issues with this and have been debugging what is going on this week. In the end we implemented a patch in our middle layer code that triggers all observers when we update the context.

If the LDClient got all the updated flags internally when changing context... shouldn't the observers also be notified? Surely this must be an important bug to fix asap?

brianmichel commented 7 months ago

@louis-launchdarkly @gwhelanLD any updates here?

brianmichel commented 7 months ago

Actually, it looks like this is resolved in the latest v9 commits? https://github.com/launchdarkly/ios-client-sdk/blob/0fedd1964fef9e3145526dfb9d13a079f588fbfb/LaunchDarkly/LaunchDarkly/LDClient.swift#L307-L312

louis-launchdarkly commented 7 months ago

Hello @patriknyblad and @brianmichel, sorry for the late reply to this issue. From the investigation, this is missed from the iOS and not some intentional behavior, and this was fixed in both https://github.com/launchdarkly/ios-client-sdk/releases/tag/8.3.1 and https://github.com/launchdarkly/ios-client-sdk/releases/tag/9.2.1

Can you please verify that this fixed your issue?

brianmichel commented 6 months ago

Hello @patriknyblad and @brianmichel, sorry for the late reply to this issue. From the investigation, this is missed from the iOS and not some intentional behavior, and this was fixed in both https://github.com/launchdarkly/ios-client-sdk/releases/tag/8.3.1 and https://github.com/launchdarkly/ios-client-sdk/releases/tag/9.2.1

Can you please verify that this fixed your issue?

@louis-launchdarkly I don't think this is fixed exactly, see https://github.com/launchdarkly/ios-client-sdk/issues/331 that I just filed

patriknyblad commented 6 months ago

@louis-launchdarkly We have been able to remove our local fix after updating the SDK. We were in contact with LD support directly and the issue is fixed for us 🙏

keelerm84 commented 4 months ago

Closing this PR as the fix from a previous commit seems to have done the trick for everyone.