Headers need to be updated to include correct nullable and nonnull for Swift

[jackfreeman]

Right now none of the headers use the nullable and nonnull keywords which can cause unexpected results when using the Branch SDK with Swift. See https://developer.apple.com/swift/blog/?id=25 for more info on these keywords and their usage.

Specifically I'm seeing a crash from initSessionWithLaunchOptions if the Branch API cannot be reached. Due to not having the nullable keyword, the params are returned as a force unwrapped optional, even though they are sometimes nil, which causes a crash when accessing them.

callbackWithParams in Branch.h should look like this to correctly integrate with Swift:

typedef void (^callbackWithParams) (nullable NSDictionary *params, nullable NSError *error);

[E-B-Smith]

Great, Thanks! I'll add this to our next release.

[aaustin]

Hey @jackfreeman - Not sure how you're seeing that? If you check out BNCCallbacks.h https://github.com/BranchMetrics/ios-branch-deep-linking/blob/master/Branch-SDK/Branch-SDK/BNCCallbacks.h, you'll see the nullable flags there. Not sure where you're observing otherwise. Can you help us understand?

[shortstuffsushi]

Hey guys, want to add notes to your updates here. The callbackWithParams and callbackWithBranchUniversalObject both mark their non-error properties as _Nonnull, but in actual use, they're frequently nil. I'm not writing an app in Swift, but this likely causes the crashes described above. I was actual coming here to note that you're calling "non-nullable" params with nil, but it seems this guy is running into it already.

Example:

Successful init calls getLatestReferringBranchUniversalObject, which returns nil unless the open was actually triggered by a link click.

However, failed inits return two empty objects, which is perhaps more unusual to handle. It might just be easier to make it nullable and return nil when appropriate, and not force empty objects in the case of errors.

[aaustin]

Thanks Graham! Interesting - do you see the nullability path for the majority use case (callbackWithParams)? I see the getLatstReferringBranchUniversalObject flow.

[jackfreeman]

I must have been looking at an older version of the headers, as I do now see the nullable flags.

As mentioned by @shortstuffsushi though, it looks like the flag is set to _Nonnull when it can in fact be nil, so that flag should be changed to _Nullable

[shortstuffsushi]

I only found one instance of params being nil in a callbackWithParams, but instantiating an empty dictionary in any error case still seems gross. (ref a bunch of callbacks). A lot of places rely on like latestReferringParams and other things from the preference helper. That seems like it could lead to nil as well, if the value is empty. I can't remember if that scenario is possible.

[shortstuffsushi]

When I wrote these in the past, I wrote them with the expectation that the params would be nullable, in a time before that concept existed, or at least was popular enough that I took the time to incorporate it. I also at the time wanted to separate it out into two callbacks, which still hasn't been done (not sure if I ever shared that plan with anyone). I'd much rather see it be a success function, where non-nullable is accurate, and would always be the case, and separately a failure function, where you always have an error to work with. Kind of like the promises paradigm of javascript.

[E-B-Smith]

Fixed in SDK release 0.12.18.