Yermo / nativescript-mapbox

:statue_of_liberty: :tokyo_tower: :mount_fuji: Native OpenGL powered Maps, by Mapbox
MIT License
194 stars 94 forks source link

Project status update #328

Open dmytro-gokun opened 5 years ago

dmytro-gokun commented 5 years ago

@EddyVerbruggen

I know you a very busy guy, but can you give us some update, please? There are quite a lot folks trying to use this plugin in their projects. But it has dozens of issues and the repository looks abandoned. There are crashes, hangs, multiple maps do not really work, ancient SDKs are used and so on. But PRs are not accepted, bugs are not fixed and so on. I do not think this makes a good impression of the plugin itself and of the NativeScript ecosystem in general (a fresh example here: https://github.com/EddyVerbruggen/nativescript-mapbox/issues/324#issuecomment-525617001).

You are a busy guy, but so are we all here. We come here, try it, face issues and move away either forking it and trying to fix it or even abandoning mapbox/NS altogether. That's such a pity situation. If you do not have any plans supporting this project - make an announcement, maybe assign a maintainer(s) and so on. It just does not make any sense to let it die. It does not deserve that fate.

EddyVerbruggen commented 5 years ago

There's no announcement nor abandonment.

The problem with the Mapbox SDKs is they change the API surface a lot for every major release and I constantly have to reinvent how to embed it in NativeScript. The last version I updated to caused crashes on Android I couldn't fix at the time and no one seems to take the time to take a stab at it either.

And of course, I'd be very happy if you or anyone else would help to maintain this repo. Heck, I would be happy to see anyone else maintain it but as usual with FOSS work for every gazillion of users only 1 or 2 will contribute - and that's not sustainable.

dmytro-gokun commented 5 years ago

Well, you see if crashes are not fixed in the latest release for 8 months... i mean what that "abandonment" word mean then? :)

The problem with the Mapbox SDKs is they change the API surface a lot for every major release and I constantly have to reinvent how to embed it in NativeScript.

This is true. A good e2e test suit would make this much easier, i think.

I'd be very happy if you or anyone else would help to maintain this repo.

I'd be happy to work on this particular issue (or any other) and contribute a PR (or ten). But I look at the repository and see PRs open for half a year w/o a comment from you, lots of issues reported w/o a comment from you and so on. Then i check my deadline and realize that i cannot release my app in time with this slow cadence. And I move on, create my own private plugin, make sure the methods i need work properly and that's it. Some ppl would not even do that, they will simply abandon mapbox/NS. So, I guess, if you'd like to see more PRs and help from other ppl, you need to classify known bugs, make a roadmap, may be mark some bugs as "Up for grabs" etc. Otherwise it will just die.

If you are willing to start this process - let me know and i will try to work on that crash. I have a couple of ideas.

EddyVerbruggen commented 5 years ago

It's a bit strange this all seems to boil down to me having to do things. In a perfect world I'd love to but god knows I can't find the time to do so. Surely, if a PR was easy to integrate and worked without issues then, of course, it would be merged. There are only 2 PRs open atm and neither of them address the more pressing issue (the crashes on Android) so I don't want to add more complexity before that's fixed.

I don't see how an e2e test suite would make upgrading the Android SDKs any easier. There's a demo app which has a lot of features and it's easy for me to manually test it, but that's not the problem at all. It's mostly with lifecycle events and how the ones of NativeScript tie into the Mapbox ones. Those have changes a number times over the last few years. And also location tracking has changes numerous times. After every SDK update, once TS compiles again and the demo app doesn't crash when pausing/resuming the app, you can be pretty sure most of the plugin works as expected and that can be checked by using the other features of the demo app. I've never had to spend any serious amount of time on functional regression issues, it's just the embedding logic and location tracking which are a PITA. On the iOS side btw, there has never been an issue which hampered progress.

And yes, of course, I'm interested in PRs. Always have been. And if you look at my other repos you can see I almost always merge them even if I have to do a lot of additional work because most of them are perfect.

Lastly, this is not a plugin I use myself (I use only ~10% of my own plugins really) but I intended to at one point and shared it on Github as usual. As this one is a real pain to maintain at times and was hard for me to make good progress on (because of nasty bugs and no effective help from the community) I had to look at other things that paid the bills etc. Doing so still makes me feel guilty about not being able to give it more attention, but it doesn't mean I don't care about it anymore.

Yermo commented 5 years ago

@EddyVerbruggen Take a look at the work I've done in my fork. This plugin is central to a big project I'm working on. I have the latests API's integrated and, at least until the latest NS upgrade, it was not crashing on Android.,

I've added a number of features and expanded the layers support to emulate "clickable layers".

I didn't send a PR because my fork diverges significantly from your work and a merge would be significant work.

I've pretty much worked through everything as I was coming up to speed taking notes as I went along.

https://github.com/Yermo/nativescript-mapbox

A merge would be a good amount of work but it might be easier to do a reverse merge ... merge the few features added to this repository into mine and then move forward from there.

If you have a moment, please take a quick look at what I've done. You'll see I've re-organized things a bit and added a ton of comments (and some questions). My goal with this approach is to make it easier to get people on board. It took significant time to come up to speed on the codebase ... my hope is that I can save others some of that effect.

EddyVerbruggen commented 5 years ago

Thanks for sharing that @Yermo! @dmytro-gokun how do you feel about trying to merge these efforts?

Yermo commented 5 years ago

@EddyVerbruggen I can merge the changes added to this repository to mine and then we can merge the whole thing together.

Over the next couple of months there are a bunch of features I need to add. My thought is to try to mirror the web-gl-js api as close as is practical to make sharing code between app and site easier.

If you like, I'd be happy to act as co-maintainer if you'd like some help.

EddyVerbruggen commented 5 years ago

@Yermo that would be awesome! I wouldn’t mind bumping a major version for once if merging would mean having a few breaking changes. Especially if that would mean this plugin gets back on track. ❤️

Yermo commented 5 years ago

It definitely looks like it'd be easier to merge into mine and then back.

The addSource/removeSource methods have been added since I forked. So far the only conflict I see is in addLayer/removeLayer methods. I added my own version since the fork and similar, but incompatible, methods were added to the source repository since.

My approach attempts to follow the mapbox-gl-js API by passing a style object in whereas the API in the source repository is:

  mapbox.addLayer(
    id: "terrain-data",  // required
    source: "terrain-source",  // id of source
    sourceLayer: "contour",  // id of layer from source
    type: "line", // supported types: circle, fill, line
    lineJoin: "round",
    lineCap: "round",
    lineColor: "#ff69b4",
    lineWidth: 1,
  );

If we adopt my approach changing the call from the above to a style specification would be a small price to pay.

It shouldn't be too bad. I'm busy this weekend but maybe I can find some time next week to merge updates in, update the angular demo I built so it works, and then I can send you the mother of all pull requests.

Yermo commented 5 years ago

I've put some hours into this but it's going to take quite a few more before I'm ready to do the merge. I'll post occasional updates.

Yermo commented 5 years ago

Current situation: https://github.com/NativeScript/android-runtime/issues/1487

If you have a chance could you take a quick look?

Yermo commented 5 years ago

@EddyVerbruggen I'm running into some issues getting the original non-angular demo to work hence the delay. NS 6 apparently changed how topmost() works and now topmost() returns undefined breaking the By Code in the original NativeScript demo.

Yermo commented 5 years ago

So I have the Nativescript demo working now. I'm still trying to track down the crash but have made some progress: https://github.com/NativeScript/NativeScript/issues/7954

paul-muckypuddle commented 5 years ago

@Yermo appreciate the regular updates - we too use this plugin but have hit similar snags - I'm afraid much of the detail of the proposed merge is above my head, but really appreciate you digging so deep.

Yermo commented 5 years ago

@paul-muckypuddle it's been a bit of a struggle as I've had to come up to speed on a level of technical weeds that I really had wanted to avoid.

I appreciate the kind words. It helps.

It looks like I have the crash on Android fixed. At least, I can't reproduce it anymore. I broke something on the iOS side I have to track down but I'm close to having the mother of all pull requests done.

There are a few API changes. Nothing major. My feeling is that the Nativescript-mapbox API should match the web-gl-js API as closely as is practical to make code sharing between mobile and web easier.

paul-muckypuddle commented 5 years ago

@Yermo that makes a lot of sense - from other, simpler projects I know the horrendous feeling of staring down into some deep tns pit as adb reports yet another exception. Got my fingers crossed for the big merge, and certain owe you a beer!

Yermo commented 5 years ago

I've sent the mother of all pull requests. https://github.com/EddyVerbruggen/nativescript-mapbox/pull/331

I have both the angular demo and the nativescript demo working on both iOS and Android. It looks like I have the Android crash resolved. I've updated the docs.

There are still some issues but I think this should represent an improvement over what's up there. I have some other work to do that will occupy me for the next few weeks.

I do plan on coming back to this because I need to implement side loading, the telemetry API, and improve the marker cluster support. All that will get done but will be a few weeks.