nitaliano / react-native-mapbox-gl

A Mapbox GL react native module for creating custom maps
Other
2.16k stars 697 forks source link

[Discussion] Moving this repo forward #1555

Closed kristfal closed 5 years ago

kristfal commented 5 years ago

Hey,

this repo has been through a lot of changes. As of now, it is a community driven project (it used to be an official Mapbox repo), and it's main maintainer is @nitaliano. I know that quite a few companies (including mine) depend on this repo.

The state of the repo is currently "okish" in my opinion. A solid piece of great work is put into it. This is really great, the repo it is production ready, but it lags far behind the official native repo (for iOS and Android) from Mapbox.

A lot of improvements that have been done in the last year for iOS and Android is not here. In addition, breaking changes from Mapbox's side is trickling in. As a result, this current repo doesn't play well with custom styles (that uses expressions) and even a few of the official basemap styles are broken. Right now, master branch has fixed a lot of outstanding issues, but we (co-maintainers) have no way to publish these to NPM.

@nitaliano have not given us clear indications of his role moving forward. We also have an open PR for 7.0.0 (#1377) which will alleviate a lot of the issues described above. This work have been lead by @nitaliano, but there have been few updates on status and ETA in the past months.

In my opinion, the community has a choice moving forward that we'll need to decide upon:

1) We wait for @nitaliano to wrap up 7.0.0 and hold back any significant improvements/changes to make the work with 7.0.0 as easy as possible

or

2) We move forward with 6.X.X, bring the current version into a better state and restart the migration to the latest native iOS and Android SDKs some time in the future.

As far as bringing the repo to a better state, there are some quite significant changes I think we should do:

a) Remove native side controls: Compass, attribution, etc. and recreate these in RN b) Remove native side map components: LocationMarker, LocationCamera logic and replace these with fully uniform RN powered components c) Reduce the surface area of native camera operations: All native side camera operations use setCamera/flyTo and we'll use the RN side to expose convenience methods such as zoomIn, zoomOut, etc. d) Reduce the surface area of native side style operations: If we can move into a state where all the heavy work of styling is done in JS and all we pass to the native side is a computed style JSON using setStyleJSON (see: https://github.com/mapbox/mapbox-gl-native/pull/9256), I think that will make things a lot easier to maintain and bring the upgrade path to 7.0.0 and beyond a lot shorter e) Generally fix bugs, improve docs and make iOS and Android behave as similar as possible

It would be super to get feedback from @nitaliano and/or @zugaldia (or anyone else from mapbox) on the choices above and who will move things forward. Silence or delaying these decisions will only make this more difficult.

It would also be great to get a list of people/companies who want to invest in this moving forward, your biggest pains and a wish list beyond what is written above.

Looking forward to your feedback,

Kristian

alexiri commented 5 years ago

IMHO, I think it might be best to concentrate on 7.0.0 (or rather, 7.2.0...). That work has to be done sooner or later anyway, so it might as well be sooner. We may not need to wait for @nitaliano if there are other capable devs that can lend a hand moving it forward.

@nitaliano, do you have anything else other than what's in #1377 now?

atomheartother commented 5 years ago

Just gonna drop in to say I appreciate the work that's been done on this repo. Our project is dependent on this module and we were on our own fork with dirty fixes for common camera problems, I'm pleasantly surprised to see how far this repo's gone since going community-driven, and we're now back to using this repo as depencency.

7.0.0 seems like a huge priority as @alexiri said, especially in terms of catching up with more recent mapbox SDKs.

sfratini commented 5 years ago

@kristfal Hey this is a great recap, kudos. Like I said before we depend on this for several projects since we just started using it so we are invested. Currently we are mostly Android developers so we can tweak Android and RN side.

As for biggest issues, right now we are using 6.1.3 and we are using the basic features and offline capabitilies but we'd love to use custom styles. They have not been reliable so we are back to just use street styles. The second part is at least for me, sometimes it feels a little unstable but maybe it was because I saw a lot of issues on the repo. And finally, documentation. It is good, but a lot of issues are just related to missing usage information. For example, the offline packs can be paused and resumed and you can only found out about that if you see and browse all the examples.

In terms of moving forward, 6.1.3 is behind almost 2 years in terms of native SDK's to I'd vote to keep going with the 7.0.0 and focus on that, make sure that we bring the latest features into RN. This is our first time collaborating like this so this might be a dumb question, so we can't close the PR ourselves? I'd fork @nitaliano work and focus on that.

ferdicus commented 5 years ago

@nitaliano have not given us clear indications of his role moving forward. We also have an open PR for 7.0.0 (#1377) which will alleviate a lot of the issues described above. This work have been lead by @nitaliano, but there have been few updates on status and ETA in the past months.

To be honest, I think it would be helpful to get clear communication on this from @nitaliano.

Either a true and honest roadmap of when this is going to be done or tell us if it's abandoned. We're all adults here, life happens things change, if 7.0 isn't happening just let us know so we can plan accordingly.

segheysens commented 5 years ago

Hi @kristfal and everyone else. First off, I want to say thank you for your contributions and belief in this SDK! I agree that it's a great tool for a wide variety of teams and projects, and want to see the repo (and community) properly supported.

I am working to discuss this with @nitaliano over the next 1-2 weeks for determining the next best steps for bringing the repository up to date. I recently joined the Mapbox Solutions Engineering team, and I have been working with a new Mapbox partner who is interested in helping to maintain the repository - we had just begun going through issues and evaluating the current state as everyone here has.

@nitaliano has put some amazing work into this library and I can't fully speak for him (beyond this comment), but having started an engineering role with a new organization, it is likely he's just been busy with onboarding as of late. He has been a great maintainer for this repo and has expressed interest in continuing his involvement.

I will continue to provide updates as I have them and thank you for your patience 🙏 If you are a regular contributor to the repository or have the interest & bandwidth to contribute, please reconfirm your interest for being involved here and I will be in touch! cc/ @zugaldia

gmaclennan commented 5 years ago

Thanks for opening this discussion. We use this repo in an app and its ongoing maintenance is important to us. I'd really like to see an update to a more recent mapbox SDK, first for the updates to gesture handling on Android, which in 6.1.3 of this library are unnatural feeling and users complain. The updates to expression handling would also be important, since it currently limits the styles we can use on mobile. Finally, we've been hitting strange bugs such as crashes after 5-10 mins of inactivity related to Telemetry when the app is offline. There seem to be several issues around this, but I am hopeful that many of these bugs might be resolved in the most recent SDK.

sfratini commented 5 years ago

@segheysens Those are good news. @nitaliano recently gave collaborator access to some people (including us) and I can confirm that we have the interest in being a part of the community.

ferdicus commented 5 years ago

@segheysens, thanks for the update! ✋ our company is heavily invested and although we're by no means android or ios cracks we would like to contribute in whichever way possible - be it issue triage or docs or monetary help.

As said before, totally understand nick being swamped with his new role. As painful as that sounds while muttering it, might it then not make more sense to move the repo to someone else, who has a dedicated chunk of time or event a group of people instead of nick?

kristfal commented 5 years ago

@segheysens appreciate the response. This is good news.

Looking at the feedback above, it seems like the main takeaways are:

1) Lots of companies heavily depend on this project, which is good. The willingness to support seems to be here. 2) Mapbox also cares, and the finding a path towards 7.0.0 will be resolved in the next few weeks in collaboration with Mapbox and @nitaliano 3) The recurring wish list here is to release an updated version of the underlying SDKs. With that in mind:

I did a review of the 7.0.0 branch changes. It contains a lot of changes, many of which tackle issues mentioned in the initial post along with updating the underlying SDKs. I wonder if it is possible to greatly reduce the scope of 7.0.0 and only update the underlying SDKs along with breaking changes that are required (mostly expressions and some underlying changes). I think most of this work, if not all of it, is done in the branch already.

@segheysens / @nitaliano, it would be great if you guys could evaluate that as a potential path forward, if you believe it will reduce the time for updating the SDKs. If this is quicker, a potential path forward could be to launch 7.0.0 with expressions only, and then follow up with minor versions adding unified controls, user location and camera changes++, and finally a v8 launch that removes the old functionality for these features. This unification work is likely easier to do for the community as well since the PRs can be much more limited in scope.

hillbillysurf commented 5 years ago

It's great to hear that this will be moving forward. We're tracking it closely. At Map Your Show, we're heavily dependent on this repo, but the recent uncertainty has led us to evaluate other options. Still, our favorite option would be continued use of this project. We would love to start contributing to this in the future too.

ericpalakovichcarr commented 5 years ago

Just wanted to reiterate I'll have a small amount of bandwidth to contribute each week. This library is a crucial part of our mobile app, so I'm ready and willing to help move things forward 👍

bartolkaruza commented 5 years ago

Hi there, I haven't contributed here before, but I just started my new job at Chargetrip, where the navigation apps rely heavily on this SDK. I expect I'll be on this repo on a regular basis and happy to help with chores and features as I get more familiar with the code base. Getting a good vibe from this community already 😄 Do you have a discord/slack channel for real time discussions?

ferdicus commented 5 years ago

hey @segheysens, any updates for us? By now this feels like a continuation of the wait we had before and the wait we had before... It's very exhausting to keep tabs on this repo and check for any meaningful update.

@nitaliano, has fallen into radio silence and it just feels awkward after all this time and with the handing over of the repo, to still not having a proper direction in which this project goes.

I'm super grateful for the work that @kristfal puts into defining some sort of roadmap, however without the blessing of @nitaliano / @zugaldia / and you it feels like wasted effort.

nitaliano commented 5 years ago

This repo is going to be moved over to a new organization called https://github.com/react-native-mapbox we have enough people now that I trust that can move this project forward without feedback from me. The idea of this library is if the community thinks something is a good idea you all should just do it and have the access to be able to do it. Creating this new open source organization will also enable us to release more libraries related to react native and mapbox and keep them under one roof.

Every owner of this new organization will have full git/npm access so we can start publishing packages again. This will also give us the chance to add more people who are serious about this project.

Once we fork the project over this weekend v7 will be merged into master with full expression support to get everyone on the newest native SDKs, and we can take it from there. I will leave this project under my github as well and it will go into maintenance mode for 6.x.x and moving forward the new organization will be the official repo.

I appreciate that you all care about what I think but you all know what you need better than I do, and I don't want to get in the way of that.

nitaliano commented 5 years ago

@bartolkaruza we have a gitter https://gitter.im/react-native-mapbox-gl/Lobby

nitaliano commented 5 years ago

The new repo link is https://github.com/react-native-mapbox/maps. I'm starting to add expressions to it now and just generally clean up the repo. Once the npm package is published this will be ready to start testing out.

Since this is a new repo we're going to start the versioning all over again so we'll continue to work towards a 1.0.0 release and everything we publish for the short term will be under 1.0.0 until we get enough feedback that this is production ready.

kristfal commented 5 years ago

Great nick! I’ll close this ticket as a path forward is found.

sfratini commented 5 years ago

@nitaliano @kristfal May I suggest something? A lot of people are already having issues already understanding that the RN repo does not reflect the same native SDK version. I think using 1.X.X will confuse people even more.

Maybe it is best to at least use the same version as the underlying SDK. In this case, if the native SDK is version 7, npm version should be 7.X.X. Either way is fine tho.

farwayer commented 5 years ago

A good idea with the same version. But how often is native library major updated? I worry that this may be the stop factor for major update of react-native version. Don't know which option is better.

alexiri commented 5 years ago

Even though it makes sense to mirror the version numbers of the SDKs, I don't think this is really possible. The Android SDK is at 7.3.0 and the iOS one is at 4.9.0. They'd have to synchronize those version numbers first.

gmaclennan commented 5 years ago

Yes it would be nice but there would not be a clear way to have breaking changes to the react-native API whilst staying on the same SDK major. I think it's important that it's clear, so maybe a table at the start of the README indicating which version corresponds to which SDK version.

sfratini commented 5 years ago

Even though it makes sense to mirror the version numbers of the SDKs, I don't think this is really possible. The Android SDK is at 7.3.0 and the iOS one is at 4.9.0. They'd have to synchronize those version numbers first.

Fair enough, I forgot about that.

nitaliano commented 5 years ago

Just an update I got through porting the iOS portion and example app last night from my old branch. I will push that to master tonight on the new repo and work on porting over the Android version tonight

ferdicus commented 5 years ago

🔥 🔥 🔥 👏 👏 👏

ferdicus commented 5 years ago

Just an update I got through porting the iOS portion and example app last night from my old branch. I will push that to master tonight on the new repo and work on porting over the Android version tonight

any updates on this @nitaliano ?

Last changes on the new repo https://github.com/react-native-mapbox/maps/commits/master were from Apr 8th and those were docs changes.

mattijsf commented 5 years ago

Since we are moving to a new repo, what the best place to create pull requests / fix issues?

nitaliano commented 5 years ago

@mattijsf the new repo

I've had some fires at work this week + a hackathon. I'll be back at updating it this weekend

alexiri commented 5 years ago

Hi @nitaliano, any news on this?

mfazekas commented 5 years ago

FWIW i've started a PR at https://github.com/react-native-mapbox/maps/pull/4 this is a rebase of @nitaliano expression branch and some improvements. I realise this migth duplicate some efforts, but we need to move with this forward ASAP.

ferdicus commented 5 years ago

A month has past @segheysens, no updates - what's up?

I, among others, have reiterated that our company is invested in this lib and that we would be happy to contribute, alas, we've heard nothing from you, @zugaldia or mapbox.

@nitaliano has morphed into beetlejuice by now...

It's sad, that @mfazekas and @kristfal need to do parallel work, because we haven't heard from API 7 in a while.

At this point, I think we could almost reopen this ticket...

zugaldia commented 5 years ago

Hey all, just a quick note to mention that development is happening on https://github.com/react-native-mapbox/maps (should this repo be archived to avoid confusion?)

Also, as explained before on https://github.com/nitaliano/react-native-mapbox-gl/issues/1238#issuecomment-461585880, this is now a community-driven project. In other words, you shouldn't wait for Mapbox to set any direction or assigning any work. Anyone should feel empowered to work on any tickets and PRs today. The only reason you'll have to tag me is if you find any upstream issues with the native Android/iOS Maps SDK - if that happens, please do let me know, I want to make sure the native SDK doesn't block any development for RN bindings here.

nitaliano commented 5 years ago

@ferdicus how much have you contributed to this project? Stop complaining about not getting free code or having me finish a job for you because you can't or don't know how to do it yourself. You've done nothing to help this repo in the past so chill out.

segheysens commented 5 years ago

Thanks for the follow-up @zugaldia and @nitaliano! @ferdicus, as I hope is clear now, @nitaliano has begun more work on top of the foundation of this repository that he and many others have contributed to over the past several years.

It is an open source project, meaning that code contributions and constructive planning input are both welcomed. If you have a question, post it. If you have an idea for a bug fix, submit a pull request. Please focus your time and efforts on anything productive - from opening complete issues with bug reports to submitting code. (Thank you @mfazekas @kristfal and others for your input already!)

*I could have communicated my piece a little better here, but after I mentioned @nitaliano will be helping to lead "next steps" conversations, I made an assumption that his and the community's recent comments made it clear that the best path forward is in the new repository. I am still coordinating at Mapbox how to set a partner up for success to regularly contribute to this new repository (this takes many conversations, outside of my day to day job), and set the right expectations with the community - in the meantime this does not restrict contributions to the repo.

If you are part of a team that uses this repository and have just been requesting status updates from the wonderful folks who have been contributing, please consider making contributions. Even if you are not a developer, here are examples of many great ways you can get involved with just a few extra minutes here and there.

segheysens commented 5 years ago

@mattijsf It makes sense to open new issues and pull requests on the new fork of the repo here.

ferdicus commented 5 years ago

@ferdicus how much have you contributed to this project? Stop complaining about not getting free code or having me finish a job for you because you can't or don't know how to do it yourself. You've done nothing to help this repo in the past so chill out.

fair enough - it was meant to be a bit incendiary to get a reply. Probably not the best approach. chilling out

And to have some grounds for some future nagging I'm gonna try to contribute some of my own 😛

sfratini commented 5 years ago

@zugaldia @segheysens We understand that this is now a community driven project. But that means that we not should expect @nitaliano to speak for Mapbox. That is when you come in. And you guys mentioned that you were going to be working on probably getting partner support and/or Mapbox involvement. And you mentioned you will get back to us in 2 weeks. So that is why I believe, even when the issue was closed, that people were expecting some sort of closure from Mapbox.

As of now, we can confirm Mapbox has zero involvement and it is fair enough, but it is also important for you guys to confirm that since you were the ones that mentioned the possibility of Mapbox support.

kristfal commented 5 years ago

@nitaliano got a small request for you. Thanks to the contribution of several community devs, 7.0.0 is now in good shape in the new repo. Thanks a lot for the groundworks you did, its a huge upgrade.

Now, we'd like to wrap things up with https://github.com/react-native-mapbox/maps/issues/31.

I'm not sure if you are reading anything in the new repo, but it would be super if you could grant membership/ownership to the react-native-mapbox NPM org so we can finish migration. Once that is done and the new package is released under the new namespace, I'll update this repo with a deprecation notice and link to the new one unless you have any objections. My NPM account username is the same as here.

kristfal commented 5 years ago

To everyone who is following this thread: We've now migrated and got V7.0.0-rc1 up on NPM.