`sharedInstance` is an optional? about appboy-ios-sdk HOT 15 CLOSED

This is the most civil disagreement on the internet ever. Should be bronzed and put I the Smithsonian.

On Feb 18, 2016, at 09:40, briancaw [email protected] wrote:

Hey @ctews

We definitely appreciate where you're coming from and we’ll think more on the points you make. We hope you can appreciate the factors we’d have to consider to make a change like this.

In the meantime, we will update our documentation (both class documentation and integration docs) to more thoroughly explain the dependence of sharedInstance: on startWithApiKey:, and we’ll include more robust code examples.

Please let us know any other comments/questions you might have, and thanks again for raising this issue and pushing the dialog forward.

Brian
—
Reply to this email directly or view it on GitHub.

from appboy-ios-sdk.

Hi Justin,

Appboy.sharedInstance() will return nil if you call it before calling the startWithApiKey: inApplication: withLaunchOptions:. We don't automatically generate an Appboy object if no API key is given. If startWithApiKey: is always the first Appboy method you call, you can force unwrap Appboy.sharedInstance() safely.

Also, an optional sharedInstance() allows us to be integrated into mParticle and Segment, which decide which SDKs to initialized on the fly.

With given reasons, we make Appboy.sharedInstance() return an optional. If you have any suggestions about how we can make things easier for you, please let us know. :)

Thanks,
Wenzhi

from appboy-ios-sdk.

Sorry for the delayed response. This still seems backwards to me. For the sanity of my code implementing this, I'd much rather have a fatalError() thrown if there's no API Keys passed to startWithApiKey when I hit the sharedInstance(). The way it is right now feels like it's covering up for someone not reading the documentation. If you throw a fatalError with a link to the documentation they are going to figure out real quick they missed a step.

from appboy-ios-sdk.

Hey Justin-

I agree with the principle of failing fast and providing prompt feedback in general, but we want to ensure that even in cases of accidental error we're never throwing errors so that we can be sure we're never going to crash a production application. In addition to unintentionally errant implementations, and the Segment/mParticle wrapper use cases Wenzhi mentions, we've also had some custom implementations in the past that involve delayed initialization, or runtime selection of the API key. All of these have the potential to cause inadvertent runtime crashes if not handled perfectly in all cases, and we want to work very hard to avoid any chance Appboy causes a crash in a client app.

We are going to make a change to add warning logging if sharedInstance() is called while in an uninitialized state so that a developer watching the logs will notice it. We expect that people will also notice that the SDK is not operational pretty quickly while they're doing an initial integration.

If you want to avoid the unwrapping each time, we recommend creating a accessor in your own code that does the unwrapping and will throw a fatalError as you've suggested in the case that the response is nil.

-Bill

from appboy-ios-sdk.

Hey @justin

Following up on this - we've released 2.18.2 that implements the warning that @billmag mentioned in his post. Please let us know if you have any further comments/questions.

Thanks,
Brian

from appboy-ios-sdk.

Just to give my 2 cents to @justin, it is really weird to make the return value of a sharedInstance optional. I totally understand to make Appboy rock solid to not crash a production app, but I am not sure if you chose the right way. You kind of abuse the sharedInstance Singleton pattern here. Apart from that the code gets more messy because we either have to use optional chaining or the typical if let assertions.

I have to support justin that there should be a fatal error thrown if you want to access the sharedInstance without providing the apiKey. Combined with a proper error message any developer will hunt the problem down in 1 minute. There is the pragmatic principle of "crash early" to find implementation issues. Apart from that when optional chaining is used all the AppBoy methods won't work if there is no API key provided, which might be even more annoying and error prone and lead to wrong expectations of what's the real error.

My vote would be to use the sharedInstance pattern properly here and to throw an error if the key wasn't provided. I guess no one will integrate Appboy and upload an AppStore build without running it just one time in development mode and in that one time it will crash with a clear error message and everyone knows what to do.

from appboy-ios-sdk.

Hey @ctews,

Thanks very much for your feedback.

Let me start by explaining a valid use case that could cause a crash in a production app if guaranteed sharedInstance was nonnull. Segment.io, mParticle, and some apps independently delay Appboy initialization depending on if it's enabled or not in their backend (or enabled client-side based on some criterion). So, for example, imagine Appboy is enabled during development so that it's always being initialized in the didFinishLaunching before any sharedInstance access, and someone uses the nonnull guarantee to code against an Appboy API method. Then down the line someone or some condition disables the Appboy initialization, the app crashes.

Second, regarding how to know when something is wrong. In terms of debugging, I don't really see that big of a difference between returning nil (in which case your code could just throw a fatal error), trying to force unwrap the instance (in which case it just fails fast), and having us throw a fatal error from within our code. The main difference is that in the latter case, our code would be crashing an app for a possible valid use case (e.g. in the case of delayed initialization, the nil instance can be the signal that Appboy isn't initialized).

In short, the contract is that if you initialize before accessing sharedInstance, sharedInstance will be nonnull (we'll update our docs to explicitly state this), but IMO there are legitimate use cases where apps could operate outside of this setup. That said, if you know you've called initialize first (which is easy signal to detect, as long as it's done in the application:didFinishLaunching: method per our standard documentation), I think it's valid/easy to either:

1. Force unwrap the Appboy shared instance. For example:

Appboy.startWithApiKey("7ef72382-b92f-4258-9be9-9e19e92f3bf1", inApplication:application, withLaunchOptions:launchOptions)
Appboy.sharedInstance()!.submitFeedback("[email protected]", message: "great app!", isReportingABug: false)

2. Proxy our sharedInstance via a non-optional method or variable that you can just call. For example:

func getAppboyInstance() -> Appboy! {
  return Appboy.sharedInstance()!;
}

or

var appboyInstance : Appboy!

...

Appboy.startWithApiKey("7ef72382-b92f-4258-9be9-9e19e92f3bf1", inApplication:application, withLaunchOptions:launchOptions)
appboyInstance = Appboy.sharedInstance();

That said, we really want to ensure that our SDK is as usable and developer friendly as possible, and we're very happy to modify things where there is demand/it makes sense, but I think at this point in time we have sufficient reason to keep it as is. Please consider the cases I mentioned and if you have a viable alternative for those we'd be very happy to hear it. Thanks again for reporting and using Appboy.

• Brian

from appboy-ios-sdk.

Hey Brian,

thanks for the answer and the clarification. Because Swift provides optional chaining there would be no need of force unwraps here. I guess startWithApiKey will never fail when you try to access it, but to be double secure it will alway make sense to use the chaining with Appboy.sharedInstance()?.foo.

For me there is just one thing really really irritating. You try to take responsibility of other companies (maybe unsolid) implementations by really abusing a pattern that has been around for iOS since years. In general the sharedInstance pattern should provide an existing instance and if it's not existing it should create a valid one. I see the point that the API key needs to be provided first, which is a standard fact for all other libraries that work the way like Appboy does.

But even by seeing your kind of valid use cases, I don't understand how the use cases of some companies seem to be legit enough to abuse a common pattern that has been there for years. I can see no other company doing this even if the same use cases might exist.

And btw this is no trolling war ;) I just really want to make clear that you might take a step back again to see the bigger picture, because now every dev needs to read the docs carefully or might be irritated as @justin and I was and we need to refactor our code for some companies use case which leads to abusing a standard pattern.

If you stick to your approach I just highly recommend to show the optional chaining in the documentation, because even beginners shouldn't be educated with the force unwrap approach, it's insecure by definition ;)

from appboy-ios-sdk.

Hey @ctews

We definitely appreciate where you're coming from and we’ll think more on the points you make. We hope you can appreciate the factors we’d have to consider to make a change like this.

In the meantime, we will update our documentation (both class documentation and integration docs) to more thoroughly explain the dependence of sharedInstance: on startWithApiKey:, and we’ll include more robust code examples.

Please let us know any other comments/questions you might have, and thanks again for raising this issue and pushing the dialog forward.

-Brian

from appboy-ios-sdk.

Bumping this.
I quote "If startWithApiKey: is always the first Appboy method you call, you can force unwrap Appboy.sharedInstance() safely."
But answers from other team members seems to suggest otherwise?
Is it definitly safe or not? (i tend to not force unwrapping but i would like to be sure that i will log everything in any case).
Regards

from appboy-ios-sdk.

Hi @tirrorex

Can you link to the answers from other team members that you're referring to? Just want to take a look at them before giving a final answer.

Thanks,
Brian

from appboy-ios-sdk.

Hi, i was talking about the valid use cases you guys describe in the answers above

from appboy-ios-sdk.

Ah I see - thanks for the context @tirrorex. Indeed it's true that If startWithApiKey: is always the first Appboy method you call, you can force unwrap Appboy.sharedInstance() safely..

The mParticle/segment use case specifically is for when folks want to check if the sharedInstance() is nil because startWithApiKey might not have been called; in those integrations they don't actually call startWithApiKey themselves (the integration does).

Please let me know if if I can provide ay other context/information here.

Thanks,
Brian

from appboy-ios-sdk.

I see, the use case is if i implement appboy in my own pod for example.
Thanks for the clarification :)

from appboy-ios-sdk.

Hi All,

We've just released 2.26.0 which introduces a new, alternative unsafeInstance non-optional method to get the Appboy singleton. This gives folks options to access our singleton in a way that is accurate and won't introduce unexpected errors for integrators (existing, upgrading, or future).

This signal has been extremely useful - thanks very much to everyone involved. We're excited about this change and look forward to continuing to build and iterate upon our API to ensure it is intuitive, safe, and robust.

Thanks,
Brian

from appboy-ios-sdk.