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

Add null Activity check in PlayServicesLocationManager.getCurrentLocationData #220

Merged
merged 2 commits into from
Nov 24, 2022

Conversation

samzmann
Copy link
Contributor

@samzmann samzmann commented Nov 15, 2022

Overview

This PR addresses issue #217 in which app is crashing probably because activity returned by mReactContext.getCurrentActivity() is null.

As suggested in my comment #217 (comment) I added a null check in PlayServicesLocationManager.getCurrentLocationData.

If currentActivity is returned as null, we invoke the error callback with a new PositionError (ACTIVITY_NULL), and then just return to end the method execution.

Questions

  1. I added the new ACTIVITY_NULL error to the PositionError class. But this is not a position error, more some underlying Android/React Native error (I don't know 😅). Adding this to PositionError is quick and easy, but to be precise and clean it would make sense to put it in another class, eg. UnexpectedError or something like that..
    Also this breaks the so called "feature parity with iOS", and makes the GeolocationError TypeScript type inaccurate.
    Any thoughts on that?

  2. At first I tried to call the error callback with emitError, but the error was not reaching the JS. So I changed to error.invoke, and then the error was reaching the JS. I see that you made changes to how the error callback is called in the last two commit (741a353, 362bf80), and I am wondering if these changes are maybe a mistake?

  3. Unrelated: I tried to run the e2e tests, but noticed the script does not exist in package.json. Maybe CONTRIBUTING.md should be updated?

Test Plan

I added the changes in my app with a patch and then hardcoded

- Activity currentActivity = mReactContext.getCurrentActivity();
+ Activity currentActivity = null;

to make sure that the null check was working and that the error was reaching the JS, which it did.

Then, I changed it back to what it's supposed to be

+ Activity currentActivity = mReactContext.getCurrentActivity();
- Activity currentActivity = null;

and made sure the location data was reaching the JS, which it did.

@michalchudziak
Copy link
Owner

Hey, thank you for the contribution! The code looks nice. I'll test it out and include it in the upcoming release.

@michalchudziak michalchudziak merged commit b6d3a06 into michalchudziak:master Nov 24, 2022
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