Skip to content
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

Bunch of iOS (+1 java) plugin issues that still need to be addressed #80

Closed
pragunvohra opened this issue Mar 24, 2016 · 6 comments
Closed

Comments

@pragunvohra
Copy link
Contributor

BranchSDK.m - unresolved issues:

  1. Only a single BranchUniversalObject on the class (self.branchUniversalObj), which means, as with Java, multiple calls to CreateBranchUniversalObject will clobber prior calls to that method.
  2. setDebug does not resolve a promise resolve setDebug promise on Android & ios with passed-in value #86
  3. Memory leaks (e.g. [NSString alloc] will need to be released)
  4. userCompletedAction still needs a way to resolve a promise depending on if the http call succeeded / via a listener; it returns too early right now

I did not (really) look at the following methods, but if I found anything from a cursory glance that I have not addressed, it's listed here (I also didn't check to ensure that we were returning the proper objects instead of just JSON strings, and whether those objects were consistent with the Java implementation):

  1. getAutoInstance
  2. registerDeepLinkController
  3. redeemRewards
  4. initWithCanonicalIdentifier
  5. initWithTitle
  6. addMetadata - does not resolve a promise
  7. registerView - does not resolve a promise
  8. showShareSheet - does not resolve a promise
  9. listOnSpotlight

BranchSDK.java - unresolved issues:

  1. setDebug does not resolve a promise
  2. userCompletedAction still needs a way to resolve a promise depending on if the http call succeeded / via a listener; it returns too early right now
@renesansz
Copy link
Contributor

@pragunvohra ,
Thanks for the report, however I think I'll be needing my colleague here to fix the issues for iOS.

In regards to userCompletedAction (Android). I think this should be fixed first on the Android SDK itself, since there is no other way to resolve the promise being returned to early.
I'll consult first to the other developers for this plugin, if they have solution for this issue.

@pragunvohra
Copy link
Contributor Author

No problem.
Re: userCompletedAction -- that's correct; userCompletedAction will need a new overload on both the Android & iOS SDKs with a callback handler.

@jestoniyap
Copy link
Contributor

@pragunvohra This is currently in progress. We'll let you know once the changes are available.

@aaustin
Copy link
Contributor

aaustin commented Apr 4, 2016

@pragunvohra We're ready to merge in #92. Take a gander if you get a chance. Thanks!

@pragunvohra
Copy link
Contributor Author

@aaustin - just did, sorry I hadn't seen @jestoniyap's comment from last week.
Overall, it looks better. I would not replace boolean return values with strings though, and I think there are some possible memory leaks.
I left more detailed comments in the PR.

@aaustin
Copy link
Contributor

aaustin commented Apr 19, 2016

We're all set here, so I'm closing this issue out. Thanks for your help!

@aaustin aaustin closed this as completed Apr 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants