diff --git a/README.md b/README.md index 4a7e3ca1..c9216e12 100644 --- a/README.md +++ b/README.md @@ -252,6 +252,45 @@ function ControlledMap() { } ``` +#### When the ref is usable + +The native map is created after React commits, so `mapRef.current` is populated +before there is anything native behind it. Calls made in that window are held +and replayed, in the order they were made, as soon as the native map exists: + +```tsx +import { useEffect, useRef } from 'react'; +import { + MapView, + type Coordinate, + type MapViewRef, +} from 'react-native-better-maps'; + +function FittedMap({ points }: { points: Coordinate[] }) { + const mapRef = useRef(null); + + useEffect(() => { + // Runs before the native map exists, and still moves the camera. + mapRef.current + ?.fitToCoordinates(points, undefined, true) + .catch((error: Error) => console.warn(error.message)); + }, [points]); + + return ; +} +``` + +So no call needs `setTimeout`, a retry, or an `onMapReady` handler to be safe. +`onMapReady` reports something later and different - that the map finished +loading its tiles - and is the right hook for showing your own UI on top of a +map that has actually drawn. + +A call still waiting when the map view unmounts rejects, as does any call made +afterwards, so handle the rejection the way the example above does. Development +builds wrapped in React's `` see this on every mount: React tears +the effect down and sets it up again, the first call is rejected by that +teardown, and the second one does the work. + ## Map providers `MapView` accepts an optional `provider` prop: @@ -575,6 +614,7 @@ A coordinate that arrives as `NaN` or out of range is dropped instead of being f - An invalid `region` is ignored, and the map keeps the region it already had. - An invalid `camera` is ignored the same way, and a pitch past the range the SDKs draw is pulled back to it rather than rejected. +- `setCamera` and `animateCamera` reject an invalid camera instead of ignoring it, straight away and on both platforms - unlike a prop, they have a promise to report it on. A pitch past the drawable range is pulled back for them too. - An overlay whose coordinates, ring length or radius cannot be drawn is skipped; its neighbours still render. - Anything supplied through `region`, `camera`, the bulk `markers` prop, or a `` / `` / `` / `` child is reported through `console.warn` in development. @@ -704,6 +744,7 @@ See [example/.env.example](example/.env.example) for the supported environment v | Provider throws before rendering | Check the [supported platforms](#supported-platforms) table. `openstreetmap` and `mapbox` are reserved for future support but do not render yet. | | Expo Go does not load native maps | Use a development build after `expo prebuild`; native Nitro modules are not available in Expo Go. | | Marker animations affect gesture smoothness | For very large marker sets, prefer clustering, shorter durations, or disable marker/cluster entering animations. | +| `MapView is not mounted` from a ref call | The map view has unmounted. Calls made before the native map exists are held and replayed, so a freshly mounted map is not the cause. | ## Development diff --git a/docs/architecture.md b/docs/architecture.md index 37f38d8a..942e46da 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -92,6 +92,31 @@ Map and overlay callbacks are wired through Nitro listeners on the HybridView. C | `mapPadding` | Edge insets in density-independent pixels. Applied via `layoutMargins` (iOS) or `setPadding` (Android). | | `fitToCoordinates(coords, padding?, animated?)` | Imperative ref method; fits camera to a set of coordinates with optional padding. | +### Imperative ref readiness + +`MapViewRef` hands out a working handle during the commit that mounts the view, +which is earlier than the native map can exist. Two buffers close that gap, and +neither uses a timer: + +- **JS** — Nitro delivers the `hybridRef` view prop one JS -> UI -> JS round trip + after the mount transaction, so `MapViewCommands` (`package/src/native/mapViewCommands.ts`) + holds every call made before it arrives and replays them in call order. Calls + left waiting when the view unmounts are rejected, and later calls reject + without reaching native. `setCamera`/`animateCamera` check the camera before + it is queued, so an invalid one rejects at once instead of waiting here. +- **Android** — `MapView.getMapAsync` answers later still, so + `DeferredGoogleMap` holds camera work until the `GoogleMap` exists, and + `configureMap` drains it after replaying the `region`/`camera` props. Without + it the adapter would accept a camera call and quietly do nothing. + `fitToCoordinates` then waits once more, in `DeferredLayout`, for the map + view's first layout pass, because `newLatLngBounds` throws on a view without a + size. It rejects if the view is released before that pass comes. iOS has no + equivalent window: `MKMapView`/`GMSMapView` exist as soon as the adapter is + installed. + +`onMapReady` is a separate, later signal - the map finished loading tiles - and +is not a precondition for using the ref. + ### Platform gaps (Phase 8) - **Provider availability** — `apple` and `google` are implemented on iOS, and `google` is implemented on Android. `openstreetmap` and `mapbox` are planned provider adapters. diff --git a/example/App.tsx b/example/App.tsx index ba8cb5c5..6c3c3403 100644 --- a/example/App.tsx +++ b/example/App.tsx @@ -6,7 +6,7 @@ import { useRef, useState, type ReactNode, - type Ref, + type RefObject, } from 'react'; import { StatusBar } from 'expo-status-bar'; import { @@ -89,6 +89,18 @@ const ANIMATION_OPTIONS: AnimationOption[] = [ { id: 'none', label: 'Off', value: false }, ]; +/** + * Identity of the native map view. `MapScene` is keyed on it so its mount + * effects run exactly when the map they drive is created. + */ +function mapSceneKey( + scenario: MapScenario, + animationOption: AnimationOption, + provider: SupportedExampleProvider, +): string { + return `${scenario.id}:${animationOption.id}:${provider}`; +} + function getSupportedMapProviders(): SupportedExampleProvider[] { switch (Platform.OS) { case 'ios': @@ -514,14 +526,72 @@ const ScenarioDock = memo(function ScenarioDock({ ); }); +type MountCameraFitOptions = { + scenario: MapScenario; + mapRef?: RefObject; + mapPadding?: EdgePadding; + onResult: (result: string) => void; +}; + +/** + * Fits the camera to the scenario markers from a mount effect - the earliest a + * consumer can reach the ref, and earlier than the native map can exist. The + * promise result is reported verbatim so a silent failure cannot hide. + */ +function useMountCameraFit({ + scenario, + mapRef, + mapPadding, + onResult, +}: MountCameraFitOptions) { + useEffect(() => { + if (scenario.advanced?.fitToCoordinatesOnMount !== true) { + return; + } + + const coordinates = (scenario.markers ?? []).map( + (marker) => marker.coordinate, + ); + const handle = mapRef?.current; + if (handle == null) { + onResult('Mount fit · no handle'); + return; + } + + onResult('Mount fit · pending'); + // A scene that is swapped out while the call is in flight must not report + // its own rejection over the incoming scene's status. + let isCurrentScene = true; + handle + .fitToCoordinates(coordinates, mapPadding, true) + .then(() => { + if (isCurrentScene) { + onResult('Mount fit · resolved'); + } + }) + .catch((error: Error) => { + if (isCurrentScene) { + onResult(`Mount fit · rejected: ${error.message}`); + } + }); + + return () => { + isCurrentScene = false; + }; + // Mount only: this scene is keyed to the native map view it drives. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); +} + type MapSceneProps = { - ref?: Ref; + ref?: RefObject; scenario: MapScenario; provider: SupportedExampleProvider; mapType: MapType; mapPadding?: EdgePadding; animationOption: AnimationOption; onMapReady: () => void; + onMountFitResult: (result: string) => void; onClusterPress: (markerIds: string[], coordinate: Coordinate) => void; onMarkerPress: (id: string) => void; onMarkerDragEnd: (id: string, coordinate: Coordinate) => void; @@ -541,6 +611,7 @@ const MapScene = memo(function MapScene({ mapPadding, animationOption, onMapReady, + onMountFitResult, onClusterPress, onMarkerPress, onMarkerDragEnd, @@ -551,6 +622,13 @@ const MapScene = memo(function MapScene({ onRegionChange, onRegionChangeComplete, }: MapSceneProps) { + useMountCameraFit({ + scenario, + mapRef: ref, + mapPadding, + onResult: onMountFitResult, + }); + const commonMapProps = { style: styles.map, mapType, @@ -584,7 +662,6 @@ const MapScene = memo(function MapScene({ return ( - ); + return ; }); type StatusHeaderProps = { @@ -841,9 +911,16 @@ export default function App() { [], ); + const handleMountFitResult = useCallback((result: string) => { + setStatus(result); + }, []); + const handleMapReady = useCallback(() => { setMapReady(true); - setStatus(scenario.name); + // The mount-effect result is the point of that scenario; do not bury it. + if (scenario.advanced?.fitToCoordinatesOnMount !== true) { + setStatus(scenario.name); + } if ( scenario.advanced?.fitToCoordinatesOnReady && @@ -898,12 +975,14 @@ export default function App() { ) -> Unit>() + + /** + * Publishes the map and drains everything waiting for it, in call order. + * + * Ignored once [release] has happened: `getMapAsync` can deliver after the + * adapter destroyed its `MapView`, and that map must not come back to life. + */ + fun attach(map: GoogleMap) { + runOnMain { + if (isReleased) { + return@runOnMain + } + + this.map = map + drain(Result.success(map)) + } + } + + /** Drops the map for good and rejects everything waiting for it, now or later. */ + fun release() { + runOnMain { + isReleased = true + map = null + drain(Result.failure(IllegalStateException(MAP_RELEASED_MESSAGE))) + } + } + + /** + * Resolves with the result of [block], running it as soon as the map exists. + * + * Rejects with whatever [block] throws, and with an [IllegalStateException] + * once [release] has happened - a released map never arrives, so the caller + * is never left waiting on one. + */ + fun promise(block: (GoogleMap) -> T): Promise { + val promise = Promise() + + whenAvailable { result -> + result + .mapCatching(block) + .onSuccess { value -> promise.resolve(value) } + .onFailure { error -> promise.reject(error) } + } + + return promise + } + + /** + * Like [promise], but for work that finishes in a later native callback: the + * promise settles when [block] calls the completion it is handed, not when + * [block] returns. + * + * A camera update that has to wait for the view's first layout pass finishes + * long after the call that scheduled it, and resolving before it ran would + * report success for a camera that has not moved. + */ + fun promiseCompletion(block: (GoogleMap, complete: (Result) -> Unit) -> Unit): Promise { + val promise = Promise() + + whenAvailable { result -> + // Everything funnels through `complete`, so the promise settles exactly + // once whether the work finished, failed, or never started. + var isSettled = false + val complete: (Result) -> Unit = { outcome -> + if (!isSettled) { + isSettled = true + outcome + .onSuccess { promise.resolve(Unit) } + .onFailure { error -> promise.reject(error) } + } + } + + result + .mapCatching { map -> block(map, complete) } + .onFailure { error -> complete(Result.failure(error)) } + } + + return promise + } + + private fun whenAvailable(deliver: (Result) -> Unit) { + runOnMain { + val currentMap = map + when { + currentMap != null -> deliver(Result.success(currentMap)) + isReleased -> deliver(Result.failure(IllegalStateException(MAP_RELEASED_MESSAGE))) + else -> waiting += deliver + } + } + } + + private fun drain(result: Result) { + val queued = waiting.toList() + waiting.clear() + for (deliver in queued) { + deliver(result) + } + } +} diff --git a/package/android/src/main/java/com/margelo/nitro/nitromaps/DeferredLayout.kt b/package/android/src/main/java/com/margelo/nitro/nitromaps/DeferredLayout.kt new file mode 100644 index 00000000..f7807a1d --- /dev/null +++ b/package/android/src/main/java/com/margelo/nitro/nitromaps/DeferredLayout.kt @@ -0,0 +1,128 @@ +package com.margelo.nitro.nitromaps + +import android.view.View +import android.view.ViewTreeObserver + +/** + * Runs work that needs [view] to have a size, holding it back until a layout + * pass gives it one: `newLatLngBounds` throws on a map view that has not been + * laid out yet. + * + * [release] cancels whatever is still waiting. A view torn down before its + * first layout pass never gets one, so without it that work would wait forever, + * along with any promise it was going to settle. + * + * The layout listener is registered only while [view] is attached to a window, + * and on that window's observer. A detached view hands out a stand-in observer + * instead, so a listener left behind when the view leaves its window - which + * React Native does before it drops the view - could no longer be taken off, + * and would keep the destroyed map alive for as long as the window lives. + * + * Main thread only, like the view it waits on. + */ +internal class DeferredLayout( + private val view: View, +) { + private class Entry( + val block: () -> Unit, + val onCancel: () -> Unit, + ) + + private val waiting = mutableListOf() + private var isReleased = false + + /** The window observer [layoutListener] is registered on, while it is. */ + private var registeredOn: ViewTreeObserver? = null + + private val layoutListener = ViewTreeObserver.OnGlobalLayoutListener { runIfLaidOut() } + + private val attachListener = + object : View.OnAttachStateChangeListener { + override fun onViewAttachedToWindow(v: View) { + startListening() + } + + override fun onViewDetachedFromWindow(v: View) { + stopListening() + } + } + + init { + view.addOnAttachStateChangeListener(attachListener) + } + + /** + * Runs [block] now if [view] already has a size, otherwise in the first + * layout pass that gives it one. [onCancel] runs instead if [release] comes + * first, and straight away once it has. + */ + fun run( + onCancel: () -> Unit = {}, + block: () -> Unit, + ) { + if (isReleased) { + onCancel() + return + } + + if (hasSize()) { + block() + return + } + + waiting += Entry(block, onCancel) + startListening() + } + + /** Stops listening for good and cancels everything still waiting, in call order. */ + fun release() { + if (isReleased) { + return + } + + isReleased = true + stopListening() + view.removeOnAttachStateChangeListener(attachListener) + for (entry in drain()) { + entry.onCancel() + } + } + + private fun runIfLaidOut() { + if (!hasSize()) { + return + } + + stopListening() + for (entry in drain()) { + entry.block() + } + } + + private fun startListening() { + if (registeredOn != null || waiting.isEmpty() || !view.isAttachedToWindow) { + return + } + + val observer = view.viewTreeObserver + observer.addOnGlobalLayoutListener(layoutListener) + registeredOn = observer + } + + private fun stopListening() { + val observer = registeredOn ?: return + registeredOn = null + // A window being torn down kills its observer, and the listener goes with it. + if (observer.isAlive) { + observer.removeOnGlobalLayoutListener(layoutListener) + } + } + + private fun drain(): List { + val drained = waiting.toList() + waiting.clear() + return drained + } + + private fun hasSize(): Boolean = view.width > 0 && view.height > 0 +} diff --git a/package/android/src/main/java/com/margelo/nitro/nitromaps/GoogleMapProviderAdapter.kt b/package/android/src/main/java/com/margelo/nitro/nitromaps/GoogleMapProviderAdapter.kt index 23807094..481594d3 100644 --- a/package/android/src/main/java/com/margelo/nitro/nitromaps/GoogleMapProviderAdapter.kt +++ b/package/android/src/main/java/com/margelo/nitro/nitromaps/GoogleMapProviderAdapter.kt @@ -4,11 +4,8 @@ import android.Manifest import android.content.ComponentCallbacks import android.content.pm.PackageManager import android.content.res.Configuration -import android.os.Handler -import android.os.Looper import android.util.Log import android.view.View -import android.view.ViewTreeObserver import androidx.annotation.Keep import androidx.core.content.ContextCompat import com.facebook.proguard.annotations.DoNotStrip @@ -23,6 +20,8 @@ import com.google.android.gms.maps.model.LatLngBounds import com.google.android.gms.maps.model.MapStyleOptions import com.margelo.nitro.core.Promise +private const val MAP_RELEASED_BEFORE_LAYOUT_MESSAGE = "MapView was released before it was laid out" + @Keep @DoNotStrip class GoogleMapProviderAdapter( @@ -38,8 +37,8 @@ class GoogleMapProviderAdapter( private var pendingPolylines: Array? = null private var pendingPolygons: Array? = null private var pendingCircles: Array? = null - private val mainHandler = Handler(Looper.getMainLooper()) private val density: Float = context.resources.displayMetrics.density + private val deferredMap = DeferredGoogleMap() private val googleMapIdAtCreation: String? = normalizeGoogleMapId(initialGoogleMapId) @@ -54,6 +53,7 @@ class GoogleMapProviderAdapter( ) private val lifecycle = MapViewLifecycleOwner(view) + private val deferredLayout = DeferredLayout(view) private var isAttachedToWindow = false @@ -119,7 +119,7 @@ class GoogleMapProviderAdapter( set(value) { _camera = value if (value != null && !isUserGesture) { - updateMapCamera(value, animated = false) + applyCameraProp(value) } } @@ -319,90 +319,75 @@ class GoogleMapProviderAdapter( syncMarkerPressHandlers() } - override fun fetchCamera(): Promise = - promiseOnMain { - googleMap?.cameraPosition?.toCamera() ?: fallbackCamera() - } + override fun fetchCamera(): Promise = deferredMap.promise { map -> map.cameraPosition.toCamera() } - /** The camera the caller last asked for, used until the map itself can answer. */ - private fun fallbackCamera(): Camera { - val camera = _camera - if (camera != null) { - return camera + override fun applyCamera(camera: Camera): Promise = + deferredMap.promise { map -> + updateMapCamera(map, camera, animated = false) } - return Camera( - center = - Coordinate( - latitude = _region?.latitude ?: 0.0, - longitude = _region?.longitude ?: 0.0, - ), - zoom = 10.0, - heading = null, - pitch = null, - altitude = null, - ) - } - - override fun applyCamera(camera: Camera) { - updateMapCamera(camera, animated = false) - } - override fun animateCamera( camera: Camera, duration: Double?, - ) { + ): Promise { val animationDuration = duration ?: 0.25 - updateMapCamera(camera, animated = true, durationMs = (animationDuration * 1000).toInt()) + return deferredMap.promise { map -> + updateMapCamera(map, camera, animated = true, durationMs = (animationDuration * 1000).toInt()) + } } - override fun getVisibleRegion(): Promise = - promiseOnMain { - googleMap?.projection?.toNitroVisibleRegion() ?: emptyVisibleRegion() - } + override fun getVisibleRegion(): Promise = deferredMap.promise { map -> map.projection.toNitroVisibleRegion() } override fun fitToCoordinates( coordinates: Array, padding: EdgePadding?, animated: Boolean?, - ) { - // Filtered before the main-thread hop: a throw out of `LatLngBounds` inside - // `runOnMain` lands on the looper, where the JS caller cannot catch it. + ): Promise { + // Filtered up front: `LatLngBounds` throws on a coordinate outside the world, + // and a skipped one deserves a warning rather than a rejected promise. val validCoordinates = coordinates.filter { it.isValid() } val skipped = coordinates.size - validCoordinates.size if (skipped > 0) { Log.w(NITRO_MAPS_LOG_TAG, "fitToCoordinates skipped $skipped coordinate(s) outside the world.") } if (validCoordinates.isEmpty()) { - return + return Promise.resolved(Unit) } - runOnMain { - val map = googleMap ?: return@runOnMain + return deferredMap.promiseCompletion { map, complete -> val builder = LatLngBounds.Builder() for (coordinate in validCoordinates) { builder.include(LatLng(coordinate.latitude, coordinate.longitude)) } val bounds = builder.build() - val runUpdate = { - // Inside the runnable: converting the insets needs the size the map was laid out with. - val target = - bounds.expandedForEdgePadding( - padding?.toPixels(density), - _mapPadding?.toPixels(density), - view.width, - view.height, - ) ?: bounds - val update = CameraUpdateFactory.newLatLngBounds(target, 0) - if (animated == true) { - map.animateCamera(update) - } else { - map.moveCamera(update) - } + // `newLatLngBounds` throws on a map that has no size yet, so the camera + // update waits for the first layout pass -- and so does the promise, which + // rejects if the view is released before that pass comes. + runWhenMapViewLaidOut( + onCancel = { + complete(Result.failure(IllegalStateException(MAP_RELEASED_BEFORE_LAYOUT_MESSAGE))) + }, + ) { + complete( + runCatching { + // Inside the callback: converting the insets needs the size the map was laid out with. + val target = + bounds.expandedForEdgePadding( + padding?.toPixels(density), + _mapPadding?.toPixels(density), + view.width, + view.height, + ) ?: bounds + val update = CameraUpdateFactory.newLatLngBounds(target, 0) + if (animated == true) { + map.animateCamera(update) + } else { + map.moveCamera(update) + } + }, + ) } - - runWhenMapViewLaidOut(runUpdate) } } @@ -543,7 +528,18 @@ class GoogleMapProviderAdapter( applyRegion(region) } } - _camera?.let { updateMapCamera(it, animated = false) } + _camera?.let { camera -> updateMapCamera(map, camera, animated = false) } + + // Last, so an imperative call made while the map was still loading wins over + // the `region`/`camera` props it was issued after. + deferredMap.attach(map) + } + + private fun applyCameraProp(camera: Camera) { + runOnMain { + val map = googleMap ?: return@runOnMain + updateMapCamera(map, camera, animated = false) + } } private fun syncMarkerPressHandlers() { @@ -635,39 +631,40 @@ class GoogleMapProviderAdapter( } } - runWhenMapViewLaidOut(runUpdate) + runWhenMapViewLaidOut(block = runUpdate) } private fun updateMapCamera( + map: GoogleMap, camera: Camera, animated: Boolean, durationMs: Int = 0, ) { - // Checked before the main-thread hop: `runOnMain` posts to the looper when called from - // anywhere else, so a throw out of `CameraPosition` would surface as an uncaught main-looper - // exception the JS caller cannot catch. + // Every camera path ends here - the `camera` prop, its replay in `configureMap`, and + // `applyCamera`/`animateCamera` - so this one check covers them all. An invalid camera is + // skipped rather than thrown: the prop path has no promise to reject, so a throw out of + // `CameraPosition` would surface as an uncaught main-thread exception. For the two + // imperative calls it is only a backstop: `MapViewRef` rejects an invalid camera in JS + // before the call is queued. if (!camera.isValid()) { Log.w(NITRO_MAPS_LOG_TAG, "Ignored an invalid camera: $camera.") return } - runOnMain { - val map = googleMap ?: return@runOnMain - val target = camera.toCameraPosition(map.cameraPosition) - if (map.cameraPosition.approximatelyEquals(target)) { - return@runOnMain - } + val target = camera.toCameraPosition(map.cameraPosition) + if (map.cameraPosition.approximatelyEquals(target)) { + return + } - val update = CameraUpdateFactory.newCameraPosition(target) - if (animated) { - if (durationMs > 0) { - map.animateCamera(update, durationMs, null) - } else { - map.animateCamera(update) - } + val update = CameraUpdateFactory.newCameraPosition(target) + if (animated) { + if (durationMs > 0) { + map.animateCamera(update, durationMs, null) } else { - map.moveCamera(update) + map.animateCamera(update) } + } else { + map.moveCamera(update) } } @@ -681,62 +678,27 @@ class GoogleMapProviderAdapter( mapView.addOnLayoutChangeListener { _, _, _, _, _, _, _, _, _ -> syncViewportSize() } - runWhenViewLaidOut(mapView, syncViewportSize) - } - - private fun runWhenMapViewLaidOut(block: () -> Unit) { - runWhenViewLaidOut(view, block) + runWhenMapViewLaidOut(block = syncViewportSize) } - private fun runWhenViewLaidOut( - target: View, + /** + * Runs [block] once the map view has a size - see [DeferredLayout]. [onCancel] runs + * instead if the map is destroyed before that. + */ + private fun runWhenMapViewLaidOut( + onCancel: () -> Unit = {}, block: () -> Unit, ) { - if (target.width > 0 && target.height > 0) { + deferredLayout.run(onCancel) { updateOverlayViewportSize() block() - return } - - target.viewTreeObserver.addOnGlobalLayoutListener( - object : ViewTreeObserver.OnGlobalLayoutListener { - override fun onGlobalLayout() { - if (target.width <= 0 || target.height <= 0) { - return - } - - target.viewTreeObserver.removeOnGlobalLayoutListener(this) - updateOverlayViewportSize() - block() - } - }, - ) } private fun updateOverlayViewportSize() { overlayController.setViewportSize(view.width, view.height) } - private fun runOnMain(block: () -> Unit) { - if (Looper.myLooper() == Looper.getMainLooper()) { - block() - } else { - mainHandler.post(block) - } - } - - private fun promiseOnMain(block: () -> T): Promise { - val promise = Promise() - runOnMain { - try { - promise.resolve(block()) - } catch (error: Throwable) { - promise.reject(error) - } - } - return promise - } - private fun handleRegionWillChange(userInteracting: Boolean) { if (userInteracting && !isUserGesture) { isUserGesture = true @@ -808,6 +770,9 @@ class GoogleMapProviderAdapter( * detaching from the window deliberately does not come here. */ private fun destroyMapView() { + deferredMap.release() + deferredLayout.release() + if (lifecycle.isDestroyed) { return } @@ -821,8 +786,3 @@ class GoogleMapProviderAdapter( } private fun normalizeGoogleMapId(value: String?): String? = value?.trim()?.takeIf { it.isNotEmpty() } - -private fun emptyVisibleRegion(): VisibleRegion { - val zero = Coordinate(latitude = 0.0, longitude = 0.0) - return VisibleRegion(nearLeft = zero, nearRight = zero, farLeft = zero, farRight = zero) -} diff --git a/package/android/src/main/java/com/margelo/nitro/nitromaps/HybridMapView.kt b/package/android/src/main/java/com/margelo/nitro/nitromaps/HybridMapView.kt index d060cc16..6efb96fb 100644 --- a/package/android/src/main/java/com/margelo/nitro/nitromaps/HybridMapView.kt +++ b/package/android/src/main/java/com/margelo/nitro/nitromaps/HybridMapView.kt @@ -288,8 +288,7 @@ class HybridMapView( override fun applyCamera(camera: Camera): Promise { val mounted = adapter ?: return notMountedRejection() - mounted.applyCamera(camera) - return Promise.resolved(Unit) + return mounted.applyCamera(camera) } override fun animateCamera( @@ -297,8 +296,7 @@ class HybridMapView( duration: Double?, ): Promise { val mounted = adapter ?: return notMountedRejection() - mounted.animateCamera(camera, duration) - return Promise.resolved(Unit) + return mounted.animateCamera(camera, duration) } override fun getVisibleRegion(): Promise { @@ -312,8 +310,7 @@ class HybridMapView( animated: Boolean?, ): Promise { val mounted = adapter ?: return notMountedRejection() - mounted.fitToCoordinates(coordinates, padding, animated) - return Promise.resolved(Unit) + return mounted.fitToCoordinates(coordinates, padding, animated) } override fun onDropView() { diff --git a/package/android/src/main/java/com/margelo/nitro/nitromaps/MapProviderAdapter.kt b/package/android/src/main/java/com/margelo/nitro/nitromaps/MapProviderAdapter.kt index e9916c9b..8432683a 100644 --- a/package/android/src/main/java/com/margelo/nitro/nitromaps/MapProviderAdapter.kt +++ b/package/android/src/main/java/com/margelo/nitro/nitromaps/MapProviderAdapter.kt @@ -45,12 +45,12 @@ interface MapProviderAdapter { fun fetchCamera(): Promise - fun applyCamera(camera: Camera) + fun applyCamera(camera: Camera): Promise fun animateCamera( camera: Camera, duration: Double?, - ) + ): Promise fun getVisibleRegion(): Promise @@ -58,7 +58,7 @@ interface MapProviderAdapter { coordinates: Array, padding: EdgePadding?, animated: Boolean?, - ) + ): Promise /** * Destroys the underlying native map and unregisters everything the adapter owns. diff --git a/package/android/src/main/java/com/margelo/nitro/nitromaps/RunOnMain.kt b/package/android/src/main/java/com/margelo/nitro/nitromaps/RunOnMain.kt new file mode 100644 index 00000000..64a1981d --- /dev/null +++ b/package/android/src/main/java/com/margelo/nitro/nitromaps/RunOnMain.kt @@ -0,0 +1,16 @@ +package com.margelo.nitro.nitromaps + +import android.os.Handler +import android.os.Looper + +private val mainHandler = Handler(Looper.getMainLooper()) + +/** Runs [block] on the main thread, inline when the caller is already there. */ +internal fun runOnMain(block: () -> Unit) { + if (Looper.myLooper() == Looper.getMainLooper()) { + block() + return + } + + mainHandler.post(block) +} diff --git a/package/src/camera/__tests__/runWithValidCamera.test.ts b/package/src/camera/__tests__/runWithValidCamera.test.ts new file mode 100644 index 00000000..69330942 --- /dev/null +++ b/package/src/camera/__tests__/runWithValidCamera.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, test } from 'bun:test'; +import { MapViewCommands } from '../../native/mapViewCommands'; +import { + INVALID_CAMERA_ERROR, + runWithValidCamera, +} from '../runWithValidCamera'; + +const validCamera = { + center: { latitude: 52.23, longitude: 21.01 }, + zoom: 12, +}; + +describe('runWithValidCamera', () => { + test('runs the call for a valid camera', async () => { + await expect( + runWithValidCamera(validCamera, async () => 'moved'), + ).resolves.toBe('moved'); + }); + + test('rejects an invalid camera without running the call', async () => { + let didRun = false; + + await expect( + runWithValidCamera( + { center: { latitude: Number.NaN, longitude: 21.01 } }, + async () => { + didRun = true; + }, + ), + ).rejects.toThrow(INVALID_CAMERA_ERROR); + expect(didRun).toBe(false); + }); + + test('rejects without queueing the call for the native map', async () => { + const commands = new MapViewCommands(); + const reached: string[] = []; + + const pending = runWithValidCamera( + { ...validCamera, zoom: Number.POSITIVE_INFINITY }, + () => + commands.run(async (calls) => { + calls.push('camera'); + }), + ); + // Had the call been buffered, the arriving handle would replay it. + commands.attach(reached); + + await expect(pending).rejects.toThrow(INVALID_CAMERA_ERROR); + expect(reached).toEqual([]); + }); + + test('leaves a finite pitch past 90 to the map, which clamps it', async () => { + await expect( + runWithValidCamera({ ...validCamera, pitch: 120 }, async () => 'moved'), + ).resolves.toBe('moved'); + }); +}); diff --git a/package/src/camera/runWithValidCamera.ts b/package/src/camera/runWithValidCamera.ts new file mode 100644 index 00000000..014e8995 --- /dev/null +++ b/package/src/camera/runWithValidCamera.ts @@ -0,0 +1,26 @@ +import type { Camera } from '../types/camera'; +import { isValidCamera } from './isValidCamera'; + +export const INVALID_CAMERA_ERROR = + 'Camera rejected: invalid center coordinate, or a zoom, heading, pitch or altitude the map cannot use'; + +/** + * Runs a `setCamera` or `animateCamera` call, or rejects it when the map cannot + * use the camera. The `camera` prop can only skip such a camera, but these + * calls have a promise to report it on, and resolving one for a camera that + * never moved the map is the silent no-op `MapViewRef` rules out. + * + * Checked before the call is queued, so it rejects straight away - also before + * the native map exists - and the same way on every platform and provider. The + * native guards behind it only skip. + */ +export function runWithValidCamera( + camera: Camera, + run: () => Promise, +): Promise { + if (!isValidCamera(camera)) { + return Promise.reject(new Error(INVALID_CAMERA_ERROR)); + } + + return run(); +} diff --git a/package/src/components/MapView.tsx b/package/src/components/MapView.tsx index 98d143dc..f4484290 100644 --- a/package/src/components/MapView.tsx +++ b/package/src/components/MapView.tsx @@ -2,12 +2,12 @@ import { useCallback, useImperativeHandle, useMemo, - useRef, type Ref, - type RefObject, } from 'react'; +import { runWithValidCamera } from '../camera/runWithValidCamera'; import { useValidCamera } from '../camera/useValidCamera'; import { useCollectedOverlays } from '../hooks/useCollectedOverlays'; +import { useMapViewCommands } from '../hooks/useMapViewCommands'; import { useNitroCallback } from '../hooks/useNitroCallback'; import { useStableValue } from '../hooks/useStableValue'; import { NativeMapView } from '../native/MapViewNative'; @@ -37,20 +37,6 @@ import type { MapViewProps, PoiPressEvent } from '../types/map'; import type { MapViewRef } from '../types/ref'; import { normalizeEnteringAnimation } from '../utils/enteringAnimation'; -const MAP_VIEW_NOT_MOUNTED_ERROR = 'MapView is not mounted'; - -function withHybridRef( - hybridRef: RefObject, - run: (hybrid: NativeMapViewHybrid) => T, -): T { - const hybrid = hybridRef.current; - if (hybrid == null) { - return Promise.reject(new Error(MAP_VIEW_NOT_MOUNTED_ERROR)) as T; - } - - return run(hybrid); -} - export function MapView({ ref, style, @@ -92,7 +78,10 @@ export function MapView({ onCirclePress: onCirclePressProp, }: MapViewProps & { ref?: Ref }) { const resolvedProvider = resolveMapProvider(provider); - const hybridRef = useRef(null); + // Both are creation-time SDK configuration, so changing either remounts the + // native view. + const nativeViewKey = `${resolvedProvider}:${googleMapId ?? ''}`; + const commands = useMapViewCommands(nativeViewKey); const { markers: collectedMarkers, polylines: collectedPolylines, @@ -173,9 +162,12 @@ export function MapView({ | ((event: PoiPressEvent) => void) | undefined; - const handleHybridRef = useCallback((nativeRef: NativeMapViewHybrid) => { - hybridRef.current = nativeRef; - }, []); + const handleHybridRef = useCallback( + (nativeRef: NativeMapViewHybrid) => { + commands.attach(nativeRef); + }, + [commands], + ); const handleMarkerPress = useCallback( (id: string) => { @@ -278,18 +270,19 @@ export function MapView({ useImperativeHandle( ref, () => ({ - getCamera: () => - withHybridRef(hybridRef, (hybrid) => hybrid.fetchCamera()), + getCamera: () => commands.run((hybrid) => hybrid.fetchCamera()), setCamera: (nextCamera) => - withHybridRef(hybridRef, (hybrid) => hybrid.applyCamera(nextCamera)), + runWithValidCamera(nextCamera, () => + commands.run((hybrid) => hybrid.applyCamera(nextCamera)), + ), animateCamera: (nextCamera, duration) => - withHybridRef(hybridRef, (hybrid) => - hybrid.animateCamera(nextCamera, duration), + runWithValidCamera(nextCamera, () => + commands.run((hybrid) => hybrid.animateCamera(nextCamera, duration)), ), getVisibleRegion: () => - withHybridRef(hybridRef, (hybrid) => hybrid.getVisibleRegion()), + commands.run((hybrid) => hybrid.getVisibleRegion()), fitToCoordinates: (coordinates, padding, animated) => - withHybridRef(hybridRef, (hybrid) => + commands.run((hybrid) => hybrid.fitToCoordinates( resolveFitCoordinates(coordinates), padding, @@ -297,12 +290,12 @@ export function MapView({ ), ), }), - [], + [commands], ); return ( new MapViewCommands(), + ); + const [commandsKey, setCommandsKey] = useState(nativeViewKey); + + if (commandsKey !== nativeViewKey) { + setCommandsKey(nativeViewKey); + setCommands(new MapViewCommands()); + } + + // Layout phase, not passive: the cleanup has to close the channel while the + // native view is still there. React removes the view before passive effects + // run, and until the channel knows it is gone a retained handle would reach + // the dead native object instead of being rejected here. + // + // It also rejects whatever the outgoing channel still held, because React + // runs this cleanup for the old instance before arming the new one. + useLayoutEffect(() => { + commands.mount(); + return () => commands.unmount(); + }, [commands]); + + return commands; +} diff --git a/package/src/native/__tests__/mapViewCommands.test.ts b/package/src/native/__tests__/mapViewCommands.test.ts new file mode 100644 index 00000000..6f856cdd --- /dev/null +++ b/package/src/native/__tests__/mapViewCommands.test.ts @@ -0,0 +1,183 @@ +import { describe, expect, test } from 'bun:test'; +import { + MAP_VIEW_NOT_MOUNTED_ERROR, + MAP_VIEW_UNMOUNTED_BEFORE_READY_ERROR, + MapViewCommands, +} from '../mapViewCommands'; + +interface FakeHybrid { + calls: string[]; +} + +function fakeHybrid(): FakeHybrid { + return { calls: [] }; +} + +function record(name: string) { + return async (target: FakeHybrid): Promise => { + target.calls.push(name); + return name; + }; +} + +describe('MapViewCommands', () => { + test('runs against the handle once it is attached', async () => { + const commands = new MapViewCommands(); + const target = fakeHybrid(); + commands.attach(target); + + await expect(commands.run(record('fit'))).resolves.toBe('fit'); + expect(target.calls).toEqual(['fit']); + }); + + test('buffers calls made before the handle arrives', async () => { + const commands = new MapViewCommands(); + const target = fakeHybrid(); + + const pending = commands.run(record('fit')); + expect(target.calls).toEqual([]); + + commands.attach(target); + + await expect(pending).resolves.toBe('fit'); + expect(target.calls).toEqual(['fit']); + }); + + test('replays buffered calls in the order they were made', async () => { + const commands = new MapViewCommands(); + const target = fakeHybrid(); + + const results = Promise.all([ + commands.run(record('first')), + commands.run(record('second')), + commands.run(record('third')), + ]); + + commands.attach(target); + + await expect(results).resolves.toEqual(['first', 'second', 'third']); + expect(target.calls).toEqual(['first', 'second', 'third']); + }); + + test('propagates a rejection from a buffered command', async () => { + const commands = new MapViewCommands(); + + const pending = commands.run(async () => { + throw new Error('native failure'); + }); + + commands.attach(fakeHybrid()); + + await expect(pending).rejects.toThrow('native failure'); + }); + + test('rejects rather than throwing when an attached command throws', async () => { + const commands = new MapViewCommands(); + commands.attach(fakeHybrid()); + + await expect( + commands.run(() => { + throw new Error('nitro argument conversion failed'); + }), + ).rejects.toThrow('nitro argument conversion failed'); + }); + + test('settles the whole buffer when one command throws instead of rejecting', async () => { + const commands = new MapViewCommands(); + const target = fakeHybrid(); + + const throwing = commands.run(() => { + throw new Error('nitro argument conversion failed'); + }); + const behind = commands.run(record('behind')); + + commands.attach(target); + + await expect(throwing).rejects.toThrow('nitro argument conversion failed'); + await expect(behind).resolves.toBe('behind'); + expect(target.calls).toEqual(['behind']); + }); + + test('replaces a stale handle when a second one is attached', async () => { + const commands = new MapViewCommands(); + const stale = fakeHybrid(); + const fresh = fakeHybrid(); + + commands.attach(stale); + commands.attach(fresh); + + await expect(commands.run(record('fit'))).resolves.toBe('fit'); + expect(stale.calls).toEqual([]); + expect(fresh.calls).toEqual(['fit']); + }); + + test('rejects buffered calls when the view unmounts first', async () => { + const commands = new MapViewCommands(); + const target = fakeHybrid(); + + const pending = commands.run(record('fit')); + commands.unmount(); + + await expect(pending).rejects.toThrow( + MAP_VIEW_UNMOUNTED_BEFORE_READY_ERROR, + ); + expect(target.calls).toEqual([]); + }); + + test('never touches the handle for a call cancelled at unmount', async () => { + const commands = new MapViewCommands(); + const target = fakeHybrid(); + + const pending = commands.run(record('fit')); + commands.unmount(); + await expect(pending).rejects.toThrow( + MAP_VIEW_UNMOUNTED_BEFORE_READY_ERROR, + ); + + commands.attach(target); + + expect(target.calls).toEqual([]); + }); + + test('rejects calls made after unmount', async () => { + const commands = new MapViewCommands(); + const target = fakeHybrid(); + commands.attach(target); + commands.unmount(); + + await expect(commands.run(record('fit'))).rejects.toThrow( + MAP_VIEW_NOT_MOUNTED_ERROR, + ); + expect(target.calls).toEqual([]); + }); + + test('keeps working through a StrictMode unmount/mount cycle', async () => { + const commands = new MapViewCommands(); + const target = fakeHybrid(); + + const discarded = commands.run(record('discarded')); + commands.unmount(); + commands.mount(); + + const kept = commands.run(record('kept')); + commands.attach(target); + + await expect(discarded).rejects.toThrow( + MAP_VIEW_UNMOUNTED_BEFORE_READY_ERROR, + ); + await expect(kept).resolves.toBe('kept'); + expect(target.calls).toEqual(['kept']); + }); + + test('keeps the handle when the effect is re-run after attaching', async () => { + const commands = new MapViewCommands(); + const target = fakeHybrid(); + + commands.attach(target); + commands.unmount(); + commands.mount(); + + await expect(commands.run(record('fit'))).resolves.toBe('fit'); + expect(target.calls).toEqual(['fit']); + }); +}); diff --git a/package/src/native/mapViewCommands.ts b/package/src/native/mapViewCommands.ts new file mode 100644 index 00000000..1759eb60 --- /dev/null +++ b/package/src/native/mapViewCommands.ts @@ -0,0 +1,108 @@ +export const MAP_VIEW_NOT_MOUNTED_ERROR = 'MapView is not mounted'; + +export const MAP_VIEW_UNMOUNTED_BEFORE_READY_ERROR = + 'MapView was unmounted before the native map became available'; + +/** + * Keeps a command's failure on the promise it returned. A native call that + * throws instead of rejecting would otherwise surface differently depending on + * whether the handle had arrived yet - the same call, caught or not caught + * purely by timing. + */ +function runCommand( + command: (target: Target) => Promise, + target: Target, +): Promise { + try { + return command(target); + } catch (error) { + return Promise.reject(error); + } +} + +interface BufferedCommand { + /** Runs the command against the arrived target and settles the caller's promise. */ + flush(target: Target): void; + /** Rejects the caller's promise without ever touching a target. */ + cancel(error: Error): void; +} + +/** + * Imperative command channel to a native view whose handle arrives late. + * + * Nitro delivers the `hybridRef` prop one JS -> UI -> JS round trip after the + * commit that mounted the view, so every call made from a consumer's mount + * effect is issued before there is anything to call. Commands made in that + * window are buffered here and replayed, in the order they were made, as soon + * as {@linkcode MapViewCommands.attach} hands over the handle. + */ +export class MapViewCommands { + private target: Target | null = null; + private buffered: BufferedCommand[] = []; + private isUnmounted = false; + + /** + * Runs {@linkcode command} against the native handle, buffering it when the + * handle has not arrived yet. + * + * Rejects with {@linkcode MAP_VIEW_NOT_MOUNTED_ERROR} once the view has + * unmounted, and with {@linkcode MAP_VIEW_UNMOUNTED_BEFORE_READY_ERROR} when + * the view unmounts while the command is still buffered. + */ + run(command: (target: Target) => Promise): Promise { + if (this.isUnmounted) { + return Promise.reject(new Error(MAP_VIEW_NOT_MOUNTED_ERROR)); + } + + const target = this.target; + if (target != null) { + return runCommand(command, target); + } + + return new Promise((resolve, reject) => { + this.buffered.push({ + flush: (arrivedTarget) => { + runCommand(command, arrivedTarget).then(resolve, reject); + }, + cancel: reject, + }); + }); + } + + /** Publishes the native handle and replays everything buffered so far. */ + attach(target: Target): void { + this.target = target; + + const buffered = this.buffered; + this.buffered = []; + for (const command of buffered) { + command.flush(target); + } + } + + /** + * Re-arms the channel after {@linkcode MapViewCommands.unmount}. StrictMode + * tears an effect down and sets it up again while the view itself stays + * mounted, so the handle survives that cycle - it belongs to the native view, + * not to the effect. + */ + mount(): void { + this.isUnmounted = false; + } + + /** Rejects every buffered command and makes later calls reject too. */ + unmount(): void { + this.isUnmounted = true; + + if (this.buffered.length === 0) { + return; + } + + const buffered = this.buffered; + this.buffered = []; + const error = new Error(MAP_VIEW_UNMOUNTED_BEFORE_READY_ERROR); + for (const command of buffered) { + command.cancel(error); + } + } +} diff --git a/package/src/types/ref.ts b/package/src/types/ref.ts index fa24206d..42e419e4 100644 --- a/package/src/types/ref.ts +++ b/package/src/types/ref.ts @@ -4,21 +4,58 @@ import type { EdgePadding, VisibleRegion } from './region'; /** * Imperative handle for controlling the map view. + * + * Every method is usable as soon as React has attached the ref. A call made + * before the native map exists - from a mount effect, for example - is held and + * replayed once it does, in the order the calls were made, so waiting for + * {@linkcode MapViewProps.onMapReady} or a timer is never necessary. + * + * A call that is still waiting when the map view unmounts rejects, as does any + * call made afterwards. Nothing silently does nothing. + * + * @example + * ```tsx + * const mapRef = useRef(null); + * + * useEffect(() => { + * mapRef.current + * ?.fitToCoordinates(points) + * .catch((error: Error) => console.warn(error.message)); + * }, []); + * ``` */ export interface MapViewRef { /** Returns the current camera position. */ getCamera(): Promise; - /** Sets the camera position immediately. */ + /** + * Sets the camera position immediately. + * + * Rejects straight away, and leaves the map where it is, for a camera the + * map cannot use: a center outside the world, or a zoom, heading, pitch or + * altitude that is `NaN`, infinite, or - for zoom and heading - too large for + * a 32-bit float. The `camera` prop skips such a camera instead. + */ setCamera(camera: Camera): Promise; - /** Animates the camera to the given position. */ + /** + * Animates the camera to the given position. Resolves once the animation has + * been handed to the native map, not when it finishes, and rejects for a + * camera the map cannot use, as {@linkcode MapViewRef.setCamera} does. + * + * @param duration Animation duration in seconds. Defaults to `0.25`. + */ animateCamera(camera: Camera, duration?: number): Promise; /** Returns the currently visible geographic region. */ getVisibleRegion(): Promise; - /** Fits the camera to show all given coordinates with optional edge padding. */ + /** + * Fits the camera to show all given coordinates with optional edge padding. + * An empty {@linkcode coordinates} list is a no-op. + * + * @param animated Pass it explicitly: omitted, iOS animates and Android jumps. + */ fitToCoordinates( coordinates: Coordinate[], padding?: EdgePadding,