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

Remove default callback function from removeExternalUserId #1291

Merged
merged 2 commits into from
Nov 10, 2021

Conversation

torufuruya
Copy link
Contributor

@torufuruya torufuruya commented Sep 27, 2021

I'm having this issue even in version 4.1.0. I looked into the code and probably found out the cause of the crash.

To address the workaround for the above issue the below PRs are created before, but I guess it should also be applied to removeExternalUserId as it also defines the default empty callback function in it and it just calls the setExternalUserId with an empty user ID internally.
#1136 #1141


This change is Reviewable

@torufuruya
Copy link
Contributor Author

torufuruya commented Oct 12, 2021

Hi. It's been more than 2 weeks. Am I missing anything to make it to be reviewed? cc: @rgomezp

@torufuruya
Copy link
Contributor Author

@emawby @rgomezp When are you able to review this PR?

@nan-li nan-li self-requested a review October 27, 2021 21:35
@nan-li
Copy link
Contributor

nan-li commented Oct 27, 2021

Thanks for sharing your findings @torufuruya, my colleagues are working on other projects so I'll take a look at this PR.

@nan-li
Copy link
Contributor

nan-li commented Nov 5, 2021

I tested this and it is fine. If no callback is given, the native code just won't invoke it.
However, I am not able to reproduce the issue you are facing (on 4.3.1).

src/index.js Show resolved Hide resolved
@nan-li nan-li self-requested a review November 10, 2021 00:51
@nan-li nan-li merged commit e1ab757 into OneSignal:main Nov 10, 2021
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.

None yet

2 participants