-
Notifications
You must be signed in to change notification settings - Fork 142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sharedInstance
is an optional?
#45
Comments
Hi Justin,
Also, an optional With given reasons, we make Thanks, |
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 |
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 |
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 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. |
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:
or
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.
|
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 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 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 ;) |
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 |
This is the most civil disagreement on the internet ever. Should be bronzed and put I the Smithsonian.
|
Bumping this. |
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, |
Hi, i was talking about the valid use cases you guys describe in the answers above |
Ah I see - thanks for the context @tirrorex. Indeed it's true that The mParticle/segment use case specifically is for when folks want to check if the sharedInstance() is nil because Please let me know if if I can provide ay other context/information here. Thanks, |
I see, the use case is if i implement appboy in my own pod for example. |
Hi All, We've just released 2.26.0 which introduces a new, alternative 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, |
Curious why
Appboy.sharedInstance()
returns an optional. I'd assume that would always be guaranteed to return a valid value. Having to unwrap it each time seems a bit insane.The text was updated successfully, but these errors were encountered: