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

Fix SDK to not use reject in SDK methods to not cause crashes #386

Merged
merged 5 commits into from
Aug 23, 2018

Conversation

guperrot
Copy link
Member

No description provided.

@guperrot guperrot changed the base branch from feature/ios-start-from-library to develop August 22, 2018 21:36
@guperrot guperrot closed this Aug 22, 2018
There is some inconsistency between our 2 native SDKs though.
@guperrot guperrot reopened this Aug 22, 2018
@guperrot guperrot changed the title Feature/no sdk crash Fix SDK to not use reject in SDK methods to not cause crashes Aug 22, 2018
dhei
dhei previously approved these changes Aug 22, 2018
@dhei dhei dismissed their stale review August 22, 2018 23:17

dismiss approval

}
promise.resolve(null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Analytics.trackEvent(event) API returns a promise actually. This will change this API to return a resolved promise containing null instead of a rejected promise containing the error. It's not quite ideal to get a resolved promise containing null. We should think more about it.

Copy link
Member

@dhei dhei Aug 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking more about it, since Analytics.trackEvent(event) API is supposed to be a void method on native SDKs, it's actually the right thing to return a resolved promise wrapping null as result. And native SDKs already logs the error and we don't need to log the same error in JS, so resolving the promise in native bridge code is the right thing to do.
My conclusion is this PR is the right change.

@dhei dhei merged commit 9665669 into develop Aug 23, 2018
@guperrot guperrot deleted the feature/no-sdk-crash branch August 23, 2018 00:01
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

Successfully merging this pull request may close these issues.

2 participants