search

nitaliano / react-native-mapbox-gl

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

Queue camera operations to be until nativeRef is available #1548

Closed mattijsf closed 5 years ago

mattijsf commented 5 years ago

Allows camera operations to be used before nativeRef is available.

Steps to reproduce:

kristfal commented 5 years ago

Any idea what the potential drawback of this is? I'd assume @nitaliano added these guards for a reason.

I know for instance we do have some potential race conditions on setCamera with bounds as a mapView inits, likely because the dimensions of the underlying view triggers wrong bounds for the viewport. Would this become worse?

mattijsf commented 5 years ago

I don't have a clear understanding of any drawbacks. However I feel that the camera API should "just work" and that the library itself needs to take care of the pending native ref challenges. It seemed to work great in my project and also for the YoYo example.

The guards have been added prior to the addition of the native ref queue so I wonder if it wasn't just a matter of @nitaliano missing the opportunity to get rid of the guards.

If the race condition you mention is reproducible we can consider having a look at https://www.npmjs.com/package/@mapbox/geo-viewport in order to convert bounds to center + zoom using the onLayout from the RN side.

I'm happy to do some further testing if you can guide me to some potential problems. Maybe @nitaliano can also have a look?

kristfal commented 5 years ago

I wasn’t aware of the pre-ref queue. Very good, this looks safe. Merging! Also agree about using geo-viewport, but iirc, it only supports integer zoom levels(?). Anyway, merging! Thanks for contributing.