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

Update for compatibility with React-Native 0.32 (Android) and fix local installation/building and local React-Native reference #502

Merged
merged 11 commits into from
Aug 30, 2016

Conversation

chrisknepper
Copy link
Contributor

This pull request seeks to address two issues with react-native-maps on Android.

  1. For a few recent versions of react-native, the constructor signature for the UI Event class changed, causing MapView to crash upon rendering. This is addressed in Update RegionChangeEvent to work with RN 0.32.0-rc #450 and that code change is also included here.
  2. The recommended installation of this plugin (as seen in Instructions.md) states that this module should be required in Gradle like this:
compile 'com.airbnb.android:react-native-maps:0.7.1'

That syntax, combined with the way that the plugin had been structured causes problems. Gradle doesn't recurse/ignores the settings.gradle in this plugin file telling it to include lib, which means that before this change, a user would also need to include this in their app's android/settings.gradle file:

include ':react-native-maps'
project(':react-native-maps').projectDir = new File(rootProject.projectDir, '../node_modules/react-native-maps/android/lib')

Which almost works, but since then it only looks in lib, it ignores the build.gradle in the plugin root telling it where to find the local react-native. These problems combine in a way which causes this plugin to download and reference react-native from Maven, not locally.

On Maven, the newest version of react-native available is 0.20.1. If Gradle gets it from there instead of locally (i.e. 0.32), it fails to build because of the signature change I refer to in point 1.

So bottom line we should reference react-nativefrom the peer node_modules folder, and encourage users of the plugin include it in their app/build.gradle files the new way:

android/app/build.gradle:

compile project(':react-native-maps')

android/settings.gradle:

include ':react-native-maps'
project(':react-native-maps').projectDir = new File(rootProject.projectDir, '../node_modules/react-native-maps/android')

…tdated version from Maven

-Update argument signature of RegionChangeEvent constructor to current react-native signature
-Temporarily define maven url override in both build.gradle files until we restructure this to remove the lib folder
@srenault
Copy link

Need this!

@Exilz
Copy link

Exilz commented Aug 26, 2016

@chrisknepper It looks great but, I'm might be wrong (I'm not very experienced in native development) but shouldn't compile 'com.facebook.react:react-native:0.32.0' in build.gradle be something like com.facebook.react:react-native:0.32+' ?
Because I can't build using 0.33-rc.0 using your fork.

Thanks for the help :)

@Exilz
Copy link

Exilz commented Aug 26, 2016

I can confirm this works by using your fork, and changing the following :

removing compile 'com.airbnb.android:react-native-maps:0.7.1' from build.gradle (using only compile project(':react-native-maps'))

changing your fork's build.gradle dependency from com.facebook.react:react-native:0.32.0 to com.facebook.react:react-native:+

Once again, I might be doing it wrong, but it might help.

@srenault
Copy link

srenault commented Aug 26, 2016

I successfully build my project by changing react-native version tocom.facebook.react:react-native:+, but now it throws me an error when my app starts:
Requiring unknown module "react".
@Exilz Do you have this error too ?

@Exilz
Copy link

Exilz commented Aug 26, 2016

No, did you properly change your package file to the fork? And then npm i again?
I also did a gradlew clean. That might help

@chrisknepper
Copy link
Contributor Author

@Exilz Yes, it may be better to use + instead of 0.32.0 to refer to react-native from the plugin. I don't know very much about Gradle, but I do know that that dependency reference has been changed in this plugin several times.

I guess I was just worried about a situation where maybe a future version of react-native changes something if we're unspecific about the version we want, it could break things. But on the other hand, react-native development moves very fast (2 week release cycle), so it may be better to change it to +.

If you all want, I can make that change here.

@Exilz
Copy link

Exilz commented Aug 26, 2016

I guess that would be the best thing to do.
react-native-maps states that : "we are not officially going to support this module on anything but the latest version of React Native" anyway 😄

…(since react-native versions increment quickly)
@chrisknepper
Copy link
Contributor Author

Done! As a side note, I'm having trouble getting my fork to install correctly on Android when using react-native link, but manual install works fine. I will try to update that today as well.

@Exilz
Copy link

Exilz commented Aug 26, 2016

Yeah I had to tweak the changes made by react-native link a bit, too.
I hope this gets merged soon 😄

@srenault
Copy link

this worked for me! thanks you too.

@chrisknepper
Copy link
Contributor Author

So testing it just now, it react-native link actually seems to work just the same as manual linking 😄

The owners of this repo made a bunch of changes to the JS, so I'll need to update my PR...

@chrisknepper
Copy link
Contributor Author

chrisknepper commented Aug 27, 2016

Okay, all resolved with the latest changes. It would be nice if the repo owners would consider merging this PR or at least acknowledging that there are people needing to use it on 0.32!

Until then, you can use my version which works by referencing react-native-maps in your project's package.json as such:

"react-native-maps": "chrisknepper/react-native-maps#3c9a6a7"

I'm glad this is helpful to people!

@brentvatne
Copy link
Contributor

Awesome @chrisknepper! I just pinged @lelandrichardson about it and he said a co-worker who is working on maps will look at this shortly.

@skellock
Copy link
Contributor

We've been trying to get a new release for a while.

For example #411 and I've got branches for RN 28, 29, and 30 to try to keep things up to date.

This repo has so much awesome in it but it's starting to age a bit.

@lelandrichardson have you considered PR this up to the React Native repo? It has a react-native-maps-shaped hole ready for this to plugin to.

If you need help, I'd be happy to assist in any way I can.

@chrisknepper
Copy link
Contributor Author

@skellock Agreed, this should ultimately be pulled into RN core and replace the iOS-only MapView since it's cross-platform and better in every way :)

@spikebrehm
Copy link

I'll take a look at this today!

GROUP=com.airbnb.android

POM_DESCRIPTION=React Native Map view component for Android
POM_URL=https://github.com/lelandrichardson/react-native-maps/tree/new-scv

Choose a reason for hiding this comment

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

These POM urls are 404ing. Does that matter? Should this point to a real URL?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this doesnt matter much, it's only used by maven central, but since the lib will be pulled from node_modules, it's never used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spikebrehm This code was moved to android/gradle.properties from android/lib/gradle.properties. I don't think that the file POM_URL points to ever existed, so it has always been 404ing, which you can see if you look at android/lib/gradle.properties in master.

I can update my PR to just remove this file entirely if you want, since it's optional.

@felipecsl
Copy link
Contributor

looks good to me

mavenLocal()
jcenter()
maven {
url "$projectDir/../../react-native/android"
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should be url "$projectDir/../node_modules/react-native/android", right? same on line 26

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually no, my change is correct. In this case, $projectDir refers to <install location>/node_modules/react-native-maps/android, so $projectDir/../../react-native/android resolves to <install location>/node_modules/react-native/android

Putting url "$projectDir/../node_modules/react-native/android" resolves to <install location>/node_modules/react-native-maps/node_modules/react-native/android, a path which may not, and probably won't exist on end-user installs of this plugin. This is one of the things which causes Gradle to fetch the package from Maven, which isn't optimal.

@@ -1,2 +1,2 @@
include ':app', ':react-native-maps'

Choose a reason for hiding this comment

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

This change caused building the example app to fail. Adding back ':app', fixed it. Once this is changed back, we can merge!

@spikebrehm
Copy link

Nevermind about this error:

npm WARN react-native-maps@0.8.0 requires a peer of react@>=15.3.0 but none was installed.
npm WARN react-native@0.32.0 requires a peer of react@~15.3.0 but none was installed.

That was from npm install in the root directory, not the example directory, which works fine. So confusing 🤦.

@chrisknepper
Copy link
Contributor Author

Done :)

@spikebrehm
Copy link

Great, thanks for your contribution! @chrisknepper 🍻.

We can always use help maintaining the Android side of this project!

@spikebrehm spikebrehm merged commit bc8084b into react-native-maps:master Aug 30, 2016
@felipecsl
Copy link
Contributor

yay thanks @chrisknepper!

@npomfret
Copy link

So in summary, with RN 0.32, what do we need to do please?

@charpeni
Copy link

@npomfret Follow installation.md, everything works fine with the latest release (0.8.0) on RN 0.32.

@chrisknepper
Copy link
Contributor Author

Thank you both @spikebrehm and @felipecsl and everyone in this thread for helping with this update 😀

I think this project is great work and I'm happy to be a part of it! IMO one of the most important aspects of this project is that it supports both platforms.

@veedeo
Copy link

veedeo commented Sep 1, 2016

After I upgraded my project to latest rn-maps Im getting this exception
am I missing something?

java.lang.NoSuchMethodError: No direct method (IJ)V in class Lcom/facebook/react/uimanager/events/Event; or its super classes (declaration of 'com.facebook.react.uimanager.events.Event' appears in /data/app/com.tandem-1/base.apk)
at com.airbnb.android.react.maps.RegionChangeEvent.(RegionChangeEvent.java:16)
at com.airbnb.android.react.maps.AirMapView$8.onCameraChange(AirMapView.java:220)
at com.google.android.gms.maps.GoogleMap$12.onCameraChange(Unknown Source)
at com.google.android.gms.maps.internal.zze$zza.onTransact(Unknown Source)
at android.os.Binder.transact(Binder.java:387)
at wd.a(:com.google.android.gms.DynamiteModulesB:93)
at maps.E.a.a(Unknown Source)
at maps.E.a$1.run(Unknown Source)
at android.os.Handler.handleCallback(Handler.java:739)
at android.os.Handler.dispatchMessage(Handler.java:95)
at android.os.Looper.loop(Looper.java:148)
at android.app.ActivityThread.main(ActivityThread.java:5417)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)

@felipecsl
Copy link
Contributor

@veedeo you're most likely not using RN 0.32.0.
Can you double check what version you're on? You can see by running ./gradlew yourapp:dependencies

@veedeo
Copy link

veedeo commented Sep 1, 2016

Im on windows, but npm list returns
+-- react-native@0.32.0
+-- react-native-maps@0.8.0

what I did to upgrade, is manually changed versions in package.json and run npm install

@felipecsl
Copy link
Contributor

you have to check what version of react native was resolved by gradle, since that's what is gonna be packaged into your apk. There is no guarantee that the one listed by npm is the right one, you have to run the gradle task I showed above in order to see it

@charpeni
Copy link

charpeni commented Sep 1, 2016

./gradlew clean maybe ?

spikebrehm pushed a commit that referenced this pull request Sep 2, 2016
The PR that added `AirMapModule` (#449) was merged after the PR that
moved all the `lib/src/*` files to just `src/*` (#502). Fix it by moving
`AirMapModule` to `src/`.
spikebrehm pushed a commit that referenced this pull request Sep 2, 2016
In #502, the local maven url pointing to the `react-native` node module
changed to work better for the example app. However, that broke the path
to `react-native` when developing in the context of the library, not the
example app. In this PR, specify both paths.
alvinthen pushed a commit to alvinthen/react-native-maps that referenced this pull request Sep 2, 2016
In react-native-maps#502, the local maven url pointing to the `react-native` node module
changed to work better for the example app. However, that broke the path
to `react-native` when developing in the context of the library, not the
example app. In this PR, specify both paths.
@christopherdro
Copy link
Collaborator

Awesome job with the new release!

Looks like a version bump was missed in one place. 😉
https://github.com/lelandrichardson/react-native-maps/blob/master/android/gradle.properties#L2

@veedeo
Copy link

veedeo commented Sep 3, 2016

@charpeni ./gradlew clean fixed the issue thanks
It feels like MapView loads much faster now, but unfortunatelly I still see the map overlapping issue

@spikebrehm
Copy link

Looks like a version bump was missed in one place.

🤦

#541

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