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 82 forks source link

LDClient.start doesn't work with a nil context #301

Closed pepas-everly closed 4 months ago

pepas-everly commented 11 months ago

After recently upgrading from 6.2.0 to 9.0.0, and switching from LDClient.start(config: config, user: nil) to LDClient.start(config: config, context: nil), it seems the completion of start never gets called.

However, if I just create a dummy context LDContextBuilder(key: "default").build() and pass that to start, it works again.

The docs indicate that if nil is passed for context, that LDClient will just create a default context for you. It appears that it not the case.

pepas-everly commented 11 months ago

To be more explicit, this works (the completion gets called):

    let config = LDConfig(mobileKey: apiEnv.launchDarklyToken, autoEnvAttributes: .disabled)
    LDClient.start(config: config, context: LDContextBuilder(key: "default").build().context!) {
        ...
    }

but this does not (the completion never gets called):

    let config = LDConfig(mobileKey: apiEnv.launchDarklyToken, autoEnvAttributes: .disabled)
    LDClient.start(config: config, context: nil) {
        ...
    }

(with this extension:)

extension Result<LDContext,ContextBuilderError> {
    var context: LDContext? {
        switch self {
        case .success(let c):
            return c
        case .failure:
            return nil
        }
    }
}
pepas-everly commented 11 months ago
Screenshot 2023-08-11 at 7 30 08 PM

https://launchdarkly.github.io/ios-client-sdk/Classes/LDClient.html

pepas-everly commented 10 months ago

Hi,

Yes, manually creating a "dummy" context is fine for our needs, so this works as a workaround.

Thanks for getting back to me!

On Mon, Aug 14, 2023 at 10:21 AM tanderson-ld @.***> wrote:

Hi @pepas-everly https://github.com/pepas-everly . Thank you for reporting this. I have also reproduced the issue. It appears to exist in 8.x as well. I have filed the bug internally as 212673. The workaround of creating your own default context is the only I can come up with at this moment. Is that workaround satisfactory for you at this moment?

— Reply to this email directly, view it on GitHub https://github.com/launchdarkly/ios-client-sdk/issues/301#issuecomment-1677537742, or unsubscribe https://github.com/notifications/unsubscribe-auth/AX25PCJVA5IP23RET7XY563XVI7ATANCNFSM6AAAAAA3NQNZVE . You are receiving this because you were mentioned.Message ID: @.***>

-- This email message may contain confidential information including Protected Health Information. If you are not the intended recipient, please notify  @.*** and delete this email.

tanderson-ld commented 10 months ago

Hi @pepas-everly. Thank you for reporting this. I have also reproduced the issue. It appears to exist in 8.x as well. I have filed the bug internally as 212673.

The workaround of creating your own context is the only I can come up with at this time. Is this workaround satisfactory for you at the moment?

However, the context you created has all the context keys set to "default". This means experiments and flag rollouts may not work correctly since all the contexts with the same key get put in the same bucket.

The following code creates an anonymous context, which is what the intended behavior is had this bug not existed. Those contexts will not show up in the dashboard since they are anonymous, but I think that is what you are used to from using version 6.2.0. Here's some more info on anonymous contexts.

let config = LDConfig(mobileKey: apiEnv.launchDarklyToken, autoEnvAttributes: .disabled)
LDClient.start(config: config, context: LDContextBuilder().build().context!) {
    ...
}
tanderson-ld commented 10 months ago

Also, as an aside, 9.x is the first version to include the Automatic Environment Attributes feature. I see in your code snippet you have autoEnvAttributes: .disabled. If you have time, would you mind sharing why this feature is not useful or desirable to you? More info on the functionality is here.

Thanks again for your help in improving the SDK!

pepas-everly commented 10 months ago

Thanks!

On Mon, Aug 14, 2023 at 10:54 AM tanderson-ld @.***> wrote:

Hi @pepas-everly https://github.com/pepas-everly. Thank you for reporting this. I have also reproduced the issue. It appears to exist in 8.x as well. I have filed the bug internally as 212673.

The workaround of creating your own context is the only I can come up with at this time. Is this workaround satisfactory for you at the moment?

However, the context you created has all the context keys set to "default". This means experiments and flag rollouts may not work correctly since all the contexts with the same key get put in the same bucket.

The following code creates an anonymous context, which is what the intended behavior is had this bug not existed. Those contexts will not show up in the dashboard since they are anonymous, but I think that is what you are used to from using version 6.2.0. Here's some more info on anonymous contexts. https://docs.launchdarkly.com/sdk/features/anonymous

let config = LDConfig(mobileKey: apiEnv.launchDarklyToken, autoEnvAttributes: .disabled) LDClient.start(config: config, context: LDContextBuilder().build().context!) { ... }

— Reply to this email directly, view it on GitHub https://github.com/launchdarkly/ios-client-sdk/issues/301#issuecomment-1677602079, or unsubscribe https://github.com/notifications/unsubscribe-auth/AX25PCOM7RLCT5LIDV62CELXVJC2DANCNFSM6AAAAAA3NQNZVE . You are receiving this because you were mentioned.Message ID: @.***>

-- This email message may contain confidential information including Protected Health Information. If you are not the intended recipient, please notify  @.*** and delete this email.

pepas-everly commented 10 months ago

would you mind sharing why this feature is not useful or desirable to you?

I planned to look into it soon, but to minimize testing effort around the upgrade, my plan was to start with it disabled, to minimize the changes caused by the upgrade.

tanderson-ld commented 10 months ago

Great! Thanks.

keelerm84 commented 4 months ago

This has been fixed in v9.4.0 and v8.4.1.