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

Use latest version of RN to build Android #547

Merged
merged 1 commit into from
Sep 8, 2016

Conversation

felipemsantana
Copy link
Contributor

Currently it's not possible to build an Android app when using RN 0.32.1 because it's looking for version 0.32.0, this commit fixes this issue.

@rt2zz
Copy link

rt2zz commented Sep 7, 2016

@FMatosS can you npm publish a new module off your fork? maintainers do not want to have compile 'com.facebook.react:react-native:+', but I think a large portion of the community would switch if there was a competing module that solves this problem.
reference: https://github.com/lelandrichardson/react-native-maps/pull/406/files#r71385422

@felipemsantana
Copy link
Contributor Author

I didn't know about this.
Other native modules do this, why they don't allow?
Could you provide a link to where this matter is discussed?

@rt2zz
Copy link

rt2zz commented Sep 7, 2016

not sure tbh but there have been several PR's to switch to the more standard 'com.facebook.react:react-native:+' form. I believe in some apps which leverage gradle heavily for dependency management this can be useful to pin the version, but in the case of most RN developers who are npm centric, it causes a lot of confusion and complication.

@GantMan
Copy link
Contributor

GantMan commented Sep 8, 2016

@rt2zz this project has wavered back and forth on this far too many times. The communication is NOT clear to the community. @lelandrichardson guys honestly have a bad rap in RN circles. I'd love to help fix this once and for all, but to be honest I'm having a hell of a time understanding what the problem/benefit tradeoff is. Do you think we can schedule up a chat to help fix this issue in an elegant way?
My twitter is @gantlaborde, we can DM private details/meeting.

@jamonholmgren
Copy link

jamonholmgren commented Sep 8, 2016

Yeah this is getting comical, let's review:

#359
#554
#547
#331
#163
#241
#502
#231
#363
#497

I probably missed some. This will get changed again, and it'll break on Android again, and we'll have the same conversation again. ¯_(ツ)_/¯

@skellock
Copy link
Contributor

skellock commented Sep 8, 2016

Make + great again.

@felipecsl felipecsl merged commit be1b40a into react-native-maps:master Sep 8, 2016
@felipecsl
Copy link
Contributor

We'll cut a new release with this change

@felipemsantana
Copy link
Contributor Author

felipemsantana commented Sep 8, 2016

They want to make the build predictable, using the same dependencies every time the Android app is built.

Since version 0.21 there's no package in Maven repository, gradle looks for the dependency in node_modules.

If I am not mistaken, to lock the dependency, you have to do it in package.json.

@felipecsl
Copy link
Contributor

yeah, I think bottom line, the fact that we use node_modules to fetch the react native dependency makes this matter more complicated, so with the goal of making things easier for everyone, let's just roll with the + and be done with this once and for all, assuming everyone is aware of the risks :)

@jamonholmgren
Copy link

Thanks @felipecsl ! We are all very grateful here. :)

@lelandrichardson
Copy link
Collaborator

To comment more generally on the things being discussed in this thread (and shout out to @GantMan for the wakeup email):

I apologize for the lack of historical maintenance of this repo, and for whatever trouble it has caused people in this thread, and others. I know you have been patient, but that patience has a limit. It's a real bummer we've caused you to reach it!

I built this project as a passion-driven side project while on vacation in Costa Rica. I thought that the MapView API in core was not as good as it could be (and also not cross platform). This served as a fun learning experience for me and, turns out, a useful library for others in the RN community. I had no idea it would get popular.

The problem is that, at the time of creating it, I had no practical use for it. I wasn't using it in any real project, and this found maintenance very difficult. Additionally, not having a solid test suite or way to detect breaking changes, maintaining a library of this size with lots of open contributions proves very very difficult.

Luckily, some others in the community were using the library in real projects, and offered to contribute to this project and help maintain it (thanks @christopherdro and @jrichardlai!).

Recently, Airbnb has started depending on this project and offers an opportunity for us to reliably maintain this project, but there are several things that we need to do first in order to make that happen. Moreover, there are a number of difficulties that have arisen from the way we use React Native at Airbnb that is different than a large subset of the RN community, which caused breaking changes in this library that we didn't realize were breaking changes (did I mention that not having a test suite makes open source difficult?)

Handling native dependencies in React Native is NOT a solved problem. There are several ways that it is done right now, and even more ways that it could be done. It is hard to support all of these methods at once, though we are getting closer.

At the moment, the following things are under way:

  1. @gilbox and @spikebrehm are working on adding multiple example projects that will be a part of this repo that will utilize different methods of dependency management, and break the build if they do not build properly.
  2. @gilbox @spikebrehm and I will be working to fix bugs that have cropped up in this library over time, and hopefully adding tests to prevent them from happening again.
  3. @gilbox is working on Google Maps support for iOS in addition to MapKit, which has been a long-asked for feature
  4. when Google Maps support is added, we will likely also add Mapbox support for both Android and iOS.
  5. We will be transferring this repository to the AIrbnb github org, where we can have more assurance that it will be actively maintained.
  6. OR, instead of (5), we will move this repository into React Native Core, where we can have more assurance that it will be actively maintained

In the meantime, if you would like to help contribute to this repo, we would love the help (and I do appreciate the fact that a lot of people have been trying to contribute, somewhat unsuccessfully, already). I would really like to see this repo succeed and get to a point of more stability. Airbnb is now at a point where it makes sense to put a bit of resources into it to make that happen, so I believe it will move in that direction.

Feel free to reach out to me via twitter or email if anyone wants to chat more about this.

cc @mkonicek @christopherdro @jrichardlai @felipecsl @gilbox @spikebrehm

@jamonholmgren
Copy link

That's amazing @lelandrichardson. Thanks so much. The Infinite Red / Ignite team is definitely willing to put some time and resources into helping you move this forward.

@spikebrehm
Copy link

Thanks @jamonholmgren! We could especially use help setting up testing in CI. Do you have any experience with that?

@GantMan
Copy link
Contributor

GantMan commented Sep 8, 2016

Yes! This is by far the best React Native maps module (even Facebook says so). I'm so happy to see this was all a growing pain and not something festering or apathetic.

Happy to help where I can!

BTW, I can help unit tests in CI. Integration tests are so fragile and are quite limited in CI. Are you going with Travis? And is there an existing issue where I should read and catch up?

@spikebrehm
Copy link

Just published 0.8.1 that includes this change.

@rt2zz
Copy link

rt2zz commented Sep 8, 2016

Awesome! Thank you everyone, most of all @lelandrichardson. This lib is incredible 🎯

@spikebrehm
Copy link

It turns out 0.8.1 was faulty, because my working directory had an additional example app containing the react-native node module, which got published as part of the NPM package (:facepalm:), causing a @providesModule error, so I've unpublished it and published 0.8.2. Use that instead!

@GantMan
Copy link
Contributor

GantMan commented Sep 8, 2016

hah, happens to us all :) I can't wait to help with tests 👍 I have a script to catch things like this in our repo, due to a similar issue.

@lelandrichardson
Copy link
Collaborator

@GantMan any contributions related to testing would be met with open arms, I assure you! We have turned on travis and are working out some testing strategies (in addition to linting, etc.) If you'd like to work on this, please do!

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.

8 participants