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

Fixed pointForCoordinate/coordinateForPoint Promises in iOS (#2039, #2061) #2150

Merged
merged 1 commit into from
May 1, 2018

Conversation

danielgindi
Copy link
Contributor

Does any other open PR do the same thing?

No

What issue is this PR fixing?

Changes to these functions that did not reflect in both providers. Currently the API is broken - expecting callback in GoogleMaps and returning Promise in Air. While the docs says it returns Promise.

How did you test this PR?

Written simple code with await to print results of both functions, in Air and Google provider.

@rborn
Copy link
Collaborator

rborn commented Apr 2, 2018

@alvelig 🐽

@lachenmayer
Copy link
Contributor

Hey there, I was actually looking at this same issue too before the holidays.
I spent a considerable amount of effort trying to get an integration test running using RCTTest.
The code for that is here: master...lachenmayer:pointforcoordinate-bug
The main difference is a new AirMapsExplorerTest target which uses RCTTestRunner to run tests. These can be run in Xcode by pressing ⌘-U in the workspace (while the packager is running).

Currently, the only test is for exactly this issue: https://github.com/lachenmayer/react-native-maps/blob/pointforcoordinate-bug/example/tests/PointForCoordinate.js
I'm going to try apply this patch & see if it passes (it should!).

In general, I was wondering if there is any interest in adding these kinds of tests - I feel like we could avoid a lot of these kinds of issues with a test suite which works very similarly to how React Native itself is tested.
If you agree, I'll make a PR with a cleaned up version of this & add some instructions on how to add new test cases.
Cheers :)

@alvelig
Copy link
Contributor

alvelig commented Apr 2, 2018

LGTM

@lachenmayer I was thinking about tests too, but did not know the best way. Let's try to add example tests for this, so we can ask contributors to make tests for their PRs.

@lachenmayer
Copy link
Contributor

@alvelig please check out #2161 for a test for this PR :)

@davija
Copy link

davija commented Apr 25, 2018

Any updates on this? It has been almost a month since there was any traffic on this and this is currently a blocking issue for me.

@rborn rborn merged commit 5f2de47 into react-native-maps:master May 1, 2018
@danielgindi danielgindi deleted the patch-2 branch May 10, 2018 06:59
This pull request was closed.
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.

5 participants