From c2f87d4928b07b4beef8243296a62f182aa4b4f5 Mon Sep 17 00:00:00 2001 From: Jakub Kasprzyk Date: Sun, 20 Sep 2026 17:54:45 +0200 Subject: [PATCH 1/2] fix: skip invalid regions and overlays instead of crashing the map An invalid `region` reached `MKCoordinateRegion` and `LatLngBounds` verbatim: MapKit raises an Objective-C NSException that Swift cannot catch, and Google Maps throws inside a Nitro view prop setter, which aborts the whole Fabric mount transaction and takes every other overlay with it. - JS: an unusable `region` is held at the last one the view accepted, with a `__DEV__` warning. It cannot become `undefined`, because React rewrites a removed prop to `null`, the optional JSI converter only short-circuits on `undefined`, and the generated struct converter then calls `asObject` on it - the same mechanism as #119. - Kotlin: `Coordinate`, `Region` and overlay descriptor validity extensions; `applyRegion` and `fitToCoordinates` check before building bounds, and `MapOverlayController` both pre-filters descriptors and treats an `IllegalArgumentException` from `GoogleMap.add*` as a skipped overlay rather than letting it unwind `reconcile`. - Swift: both provider adapters guard `applyRegion` with `CLLocationCoordinate2DIsValid` and filter `fitToCoordinates`. A span whose edges run past a pole is pulled back with `regionThatFits` - as `animateToClusterRegion` already did - instead of being rejected, and the Google bounds conversion clamps latitude for the same reason. `fitToCoordinates` filters on the calling thread, so a bad coordinate no longer reaches `LatLngBounds` on the main looper, where the throw surfaces as an uncaught exception the JS caller cannot handle. The imperative method validates in JS too, so both platforms report the skip the same way. The shared bounds rule lives in the `ios/Geometry` SPM target because that is the only iOS test target CI runs - autolinking declares the pod without `:testspecs`, so `iosTests/` is never built. Drive-by, all flagged by review: the coordinate predicates move out of `overlays/` into `utils/validateGeometry.ts` so `region/` no longer depends on them through another feature; the three identical `warn*` helpers collapse onto one `createWarn`; the logcat tag has one definition again; and the shared `console.warn` spy is cleared before each test, not only after - it leaks across files and already failed `warnOverlay` in a full-suite run. Refs #125 --- README.md | 15 +++ .../nitro/nitromaps/Coordinate+Validity.kt | 19 +++ .../nitromaps/GoogleMapProviderAdapter.kt | 17 ++- .../nitro/nitromaps/MapOverlayController.kt | 112 +++++++++++++++--- .../nitro/nitromaps/MarkerIconFactory.kt | 5 +- .../nitro/nitromaps/NitroMapsLogTag.kt | 4 + .../nitromaps/OverlayDescriptor+Validity.kt | 20 ++++ .../nitro/nitromaps/Region+Validity.kt | 9 ++ .../nitro/nitromaps/CoordinateValidityTest.kt | 51 ++++++++ .../OverlayDescriptorValidityTest.kt | 86 ++++++++++++++ .../nitro/nitromaps/RegionValidityTest.kt | 50 ++++++++ package/ios/AppleMapProviderAdapter.swift | 16 ++- package/ios/Coordinate+Validity.swift | 7 ++ package/ios/Geometry/RegionSpan.swift | 14 +++ package/ios/GoogleMapProviderAdapter.swift | 9 +- package/ios/Package.swift | 15 ++- package/ios/Region+GMSCoordinateBounds.swift | 7 +- package/ios/Region+Validity.swift | 9 ++ .../HexColorComponentsTests.swift | 0 .../ios/Tests/Geometry/RegionSpanTests.swift | 18 +++ package/src/components/MapView.tsx | 11 +- .../src/geojson/__tests__/warnGeojson.test.ts | 6 +- package/src/geojson/warnGeojson.ts | 8 +- .../overlays/__tests__/warnOverlay.test.ts | 6 +- package/src/overlays/collectOverlayChild.ts | 2 +- package/src/overlays/warnOverlay.ts | 8 +- .../region/__tests__/isValidRegion.test.ts | 54 +++++++++ .../__tests__/resolveFitCoordinates.test.ts | 58 +++++++++ .../__tests__/resolveRegionProp.test.ts | 77 ++++++++++++ package/src/region/isValidRegion.ts | 16 +++ package/src/region/resolveFitCoordinates.ts | 20 ++++ package/src/region/resolveRegionProp.ts | 22 ++++ package/src/region/useValidRegion.ts | 28 +++++ package/src/region/warnRegion.ts | 3 + .../__tests__/validateGeometry.test.ts} | 4 +- .../validateGeometry.ts} | 0 package/src/utils/warn.ts | 9 ++ 37 files changed, 765 insertions(+), 50 deletions(-) create mode 100644 package/android/src/main/java/com/margelo/nitro/nitromaps/Coordinate+Validity.kt create mode 100644 package/android/src/main/java/com/margelo/nitro/nitromaps/NitroMapsLogTag.kt create mode 100644 package/android/src/main/java/com/margelo/nitro/nitromaps/OverlayDescriptor+Validity.kt create mode 100644 package/android/src/main/java/com/margelo/nitro/nitromaps/Region+Validity.kt create mode 100644 package/android/src/test/java/com/margelo/nitro/nitromaps/CoordinateValidityTest.kt create mode 100644 package/android/src/test/java/com/margelo/nitro/nitromaps/OverlayDescriptorValidityTest.kt create mode 100644 package/android/src/test/java/com/margelo/nitro/nitromaps/RegionValidityTest.kt create mode 100644 package/ios/Coordinate+Validity.swift create mode 100644 package/ios/Geometry/RegionSpan.swift create mode 100644 package/ios/Region+Validity.swift rename package/ios/Tests/{ => ColorParser}/HexColorComponentsTests.swift (100%) create mode 100644 package/ios/Tests/Geometry/RegionSpanTests.swift create mode 100644 package/src/region/__tests__/isValidRegion.test.ts create mode 100644 package/src/region/__tests__/resolveFitCoordinates.test.ts create mode 100644 package/src/region/__tests__/resolveRegionProp.test.ts create mode 100644 package/src/region/isValidRegion.ts create mode 100644 package/src/region/resolveFitCoordinates.ts create mode 100644 package/src/region/resolveRegionProp.ts create mode 100644 package/src/region/useValidRegion.ts create mode 100644 package/src/region/warnRegion.ts rename package/src/{overlays/__tests__/validateOverlay.test.ts => utils/__tests__/validateGeometry.test.ts} (96%) rename package/src/{overlays/validateOverlay.ts => utils/validateGeometry.ts} (100%) create mode 100644 package/src/utils/warn.ts diff --git a/README.md b/README.md index 14e8d614..dfe06517 100644 --- a/README.md +++ b/README.md @@ -36,6 +36,7 @@ Built with [Nitro Modules](https://nitro.margelo.com/) for high-performance nati - [Google Maps setup](#google-maps-setup) - [Marker entering animations](#marker-entering-animations) - [Re-renders](#re-renders) +- [Invalid input](#invalid-input) - [Capability matrix](#capability-matrix) - [Public API](#public-api) - [Example app](#example-app) @@ -540,6 +541,20 @@ setMarkers((current) => ); ``` +## Invalid input + +A coordinate that arrives as `NaN` or out of range is dropped instead of being forwarded to MapKit and the Google Maps SDK, which throw on it: + +- An invalid `region` is ignored, and the map keeps the region it already had. +- An overlay whose coordinates, ring length or radius cannot be drawn is skipped; its neighbours still render. +- Anything supplied through `region` or through a `` / `` / `` / `` child is reported through `console.warn` in development. + +Where the check runs depends on the entry point. `region` and `fitToCoordinates` are guarded natively on both platforms, so a `hybridRef` call cannot reach the SDKs either. Overlay descriptors are additionally filtered natively on Android, where an undrawable overlay throws inside the Fabric mount transaction and would otherwise take the whole screen down; those skips are reported to logcat rather than `console.warn`. + +Two gaps are worth knowing about: the `camera` prop is not validated anywhere, and descriptors passed through the bulk `markers` prop are checked on neither side - only the `` child is. + +Valid means: latitude and longitude finite and within ±90 / ±180, region deltas finite and greater than 0, two coordinates for a polyline, three per polygon ring, and a finite radius of at least 0 for a circle. A region whose span would run past a pole is pulled back to what the map can show rather than rejected. + ## Capability matrix | Capability | `apple` iOS | `google` iOS | `google` Android | diff --git a/package/android/src/main/java/com/margelo/nitro/nitromaps/Coordinate+Validity.kt b/package/android/src/main/java/com/margelo/nitro/nitromaps/Coordinate+Validity.kt new file mode 100644 index 00000000..afea0fa4 --- /dev/null +++ b/package/android/src/main/java/com/margelo/nitro/nitromaps/Coordinate+Validity.kt @@ -0,0 +1,19 @@ +package com.margelo.nitro.nitromaps + +/** Google Maps throws on a coordinate it cannot place, which unwinds the Fabric mount transaction. */ +internal fun Coordinate.isValid(): Boolean = isValidCoordinate(latitude, longitude) + +/** Scalar form, so a `Region` can check its center without building a `Coordinate` for it. */ +internal fun isValidCoordinate( + latitude: Double, + longitude: Double, +): Boolean = + latitude.isFinite() && + latitude >= -90.0 && + latitude <= 90.0 && + longitude.isFinite() && + longitude >= -180.0 && + longitude <= 180.0 + +/** Minimum size is 2 for a polyline and 3 for a polygon ring. */ +internal fun Array.isValidPath(minimumSize: Int): Boolean = size >= minimumSize && all { it.isValid() } 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 3cf9ac42..7ec33a5d 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 @@ -6,6 +6,7 @@ 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 @@ -361,14 +362,21 @@ class GoogleMapProviderAdapter( padding: EdgePadding?, animated: Boolean?, ) { - if (coordinates.isEmpty()) { + // Filtered before the main-thread hop: a throw out of `LatLngBounds` inside + // `runOnMain` lands on the looper, where the JS caller cannot catch it. + 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 } runOnMain { val map = googleMap ?: return@runOnMain val builder = LatLngBounds.Builder() - for (coordinate in coordinates) { + for (coordinate in validCoordinates) { builder.include(LatLng(coordinate.latitude, coordinate.longitude)) } val bounds = builder.build() @@ -602,6 +610,11 @@ class GoogleMapProviderAdapter( region: Region, animated: Boolean = false, ) { + if (!region.isValid()) { + Log.w(NITRO_MAPS_LOG_TAG, "Ignored an invalid region: $region.") + return + } + val map = googleMap ?: return val bounds = region.toLatLngBounds() val paddingPx = _mapPadding.toPaddingPixels() diff --git a/package/android/src/main/java/com/margelo/nitro/nitromaps/MapOverlayController.kt b/package/android/src/main/java/com/margelo/nitro/nitromaps/MapOverlayController.kt index d2c4d9db..19b53deb 100644 --- a/package/android/src/main/java/com/margelo/nitro/nitromaps/MapOverlayController.kt +++ b/package/android/src/main/java/com/margelo/nitro/nitromaps/MapOverlayController.kt @@ -6,6 +6,7 @@ import android.animation.ValueAnimator import android.os.Handler import android.os.Looper import android.os.SystemClock +import android.util.Log import com.facebook.react.uimanager.ThemedReactContext import com.google.android.gms.maps.CameraUpdateFactory import com.google.android.gms.maps.GoogleMap @@ -544,17 +545,27 @@ class MapOverlayController( val map = googleMap ?: return reconcile( current = polylines, - next = descriptors?.associateBy { it.id } ?: emptyMap(), + next = + validDescriptorsById( + descriptors = descriptors, + kind = "polyline", + id = { it.id }, + isValid = { it.isValid() }, + ), remove = { it.remove() }, add = { descriptor -> - map.addPolyline(descriptor.toPolylineOptions()).also { polyline -> - polyline.tag = descriptor.id + addedOrNull(kind = "polyline", id = descriptor.id) { + map.addPolyline(descriptor.toPolylineOptions()).also { polyline -> + polyline.tag = descriptor.id + } } }, update = { polyline, descriptor -> polyline.remove() - map.addPolyline(descriptor.toPolylineOptions()).also { replacement -> - replacement.tag = descriptor.id + addedOrNull(kind = "polyline", id = descriptor.id) { + map.addPolyline(descriptor.toPolylineOptions()).also { replacement -> + replacement.tag = descriptor.id + } } }, ) @@ -564,17 +575,27 @@ class MapOverlayController( val map = googleMap ?: return reconcile( current = polygons, - next = descriptors?.associateBy { it.id } ?: emptyMap(), + next = + validDescriptorsById( + descriptors = descriptors, + kind = "polygon", + id = { it.id }, + isValid = { it.isValid() }, + ), remove = { it.remove() }, add = { descriptor -> - map.addPolygon(descriptor.toPolygonOptions()).also { polygon -> - polygon.tag = descriptor.id + addedOrNull(kind = "polygon", id = descriptor.id) { + map.addPolygon(descriptor.toPolygonOptions()).also { polygon -> + polygon.tag = descriptor.id + } } }, update = { polygon, descriptor -> polygon.remove() - map.addPolygon(descriptor.toPolygonOptions()).also { replacement -> - replacement.tag = descriptor.id + addedOrNull(kind = "polygon", id = descriptor.id) { + map.addPolygon(descriptor.toPolygonOptions()).also { replacement -> + replacement.tag = descriptor.id + } } }, ) @@ -584,28 +605,80 @@ class MapOverlayController( val map = googleMap ?: return reconcile( current = circles, - next = descriptors?.associateBy { it.id } ?: emptyMap(), + next = + validDescriptorsById( + descriptors = descriptors, + kind = "circle", + id = { it.id }, + isValid = { it.isValid() }, + ), remove = { it.remove() }, add = { descriptor -> - map.addCircle(descriptor.toCircleOptions()).also { circle -> - circle.tag = descriptor.id + addedOrNull(kind = "circle", id = descriptor.id) { + map.addCircle(descriptor.toCircleOptions()).also { circle -> + circle.tag = descriptor.id + } } }, update = { circle, descriptor -> circle.remove() - map.addCircle(descriptor.toCircleOptions()).also { replacement -> - replacement.tag = descriptor.id + addedOrNull(kind = "circle", id = descriptor.id) { + map.addCircle(descriptor.toCircleOptions()).also { replacement -> + replacement.tag = descriptor.id + } } }, ) } + /** Dropping the id also removes what it used to render, since `reconcile` treats a missing id as a removal. */ + private fun validDescriptorsById( + descriptors: Array?, + kind: String, + id: (Descriptor) -> String, + isValid: (Descriptor) -> Boolean, + ): Map { + if (descriptors == null) { + return emptyMap() + } + + val valid = LinkedHashMap(descriptors.size) + for (descriptor in descriptors) { + if (!isValid(descriptor)) { + Log.w(NITRO_MAPS_LOG_TAG, "Skipped $kind \"${id(descriptor)}\": it cannot be drawn.") + continue + } + + valid[id(descriptor)] = descriptor + } + + return valid + } + + /** + * The pre-filter only models what the descriptors declare; `GoogleMap.add*` + * can still reject a value for a reason of its own, and `reconcile` runs + * inside a view prop setter, where an escaping throw aborts the whole mount + * transaction. + */ + private fun addedOrNull( + kind: String, + id: String, + add: () -> T, + ): T? = + try { + add() + } catch (error: IllegalArgumentException) { + Log.w(NITRO_MAPS_LOG_TAG, "Skipped $kind \"$id\": the Google Maps SDK rejected it.", error) + null + } + private fun reconcile( current: MutableMap, next: Map, remove: (T) -> Unit, add: (Descriptor) -> T?, - update: (T, Descriptor) -> T, + update: (T, Descriptor) -> T?, ) { val nextIds = next.keys val existingIds = current.keys @@ -621,7 +694,12 @@ class MapOverlayController( current[id] = created } } else { - current[id] = update(existing, descriptor) + val updated = update(existing, descriptor) + if (updated == null) { + current.remove(id) + } else { + current[id] = updated + } } } } diff --git a/package/android/src/main/java/com/margelo/nitro/nitromaps/MarkerIconFactory.kt b/package/android/src/main/java/com/margelo/nitro/nitromaps/MarkerIconFactory.kt index c44d27d9..884fc6d5 100644 --- a/package/android/src/main/java/com/margelo/nitro/nitromaps/MarkerIconFactory.kt +++ b/package/android/src/main/java/com/margelo/nitro/nitromaps/MarkerIconFactory.kt @@ -258,7 +258,7 @@ internal class MarkerIconFactory( cacheBitmap(key, resizeBitmap(bitmap, image)) } } catch (error: Exception) { - Log.w(TAG, "Failed to load marker image: ${image.uri}", error) + Log.w(NITRO_MAPS_LOG_TAG, "Failed to load marker image: ${image.uri}", error) null } } @@ -503,7 +503,7 @@ internal class MarkerIconFactory( uri: String, reason: String, ) { - Log.w(TAG, "Rejected remote marker image URI ($reason): $uri") + Log.w(NITRO_MAPS_LOG_TAG, "Rejected remote marker image URI ($reason): $uri") } private fun resizeBitmap( @@ -523,7 +523,6 @@ internal class MarkerIconFactory( } private companion object { - const val TAG = "NitroMaps" const val DEFAULT_ICON_KEY = "__default__" private const val DEFAULT_MARKER_WIDTH_DP = 40f private const val DEFAULT_MARKER_HEIGHT_DP = 52f diff --git a/package/android/src/main/java/com/margelo/nitro/nitromaps/NitroMapsLogTag.kt b/package/android/src/main/java/com/margelo/nitro/nitromaps/NitroMapsLogTag.kt new file mode 100644 index 00000000..c6726ac4 --- /dev/null +++ b/package/android/src/main/java/com/margelo/nitro/nitromaps/NitroMapsLogTag.kt @@ -0,0 +1,4 @@ +package com.margelo.nitro.nitromaps + +/** Single logcat tag for the whole module. */ +internal const val NITRO_MAPS_LOG_TAG = "NitroMaps" diff --git a/package/android/src/main/java/com/margelo/nitro/nitromaps/OverlayDescriptor+Validity.kt b/package/android/src/main/java/com/margelo/nitro/nitromaps/OverlayDescriptor+Validity.kt new file mode 100644 index 00000000..f1757907 --- /dev/null +++ b/package/android/src/main/java/com/margelo/nitro/nitromaps/OverlayDescriptor+Validity.kt @@ -0,0 +1,20 @@ +package com.margelo.nitro.nitromaps + +/** A polyline needs two placeable points before `GoogleMap.addPolyline` accepts it. */ +internal fun PolylineDescriptor.isValid(): Boolean = coordinates.isValidPath(MINIMUM_POLYLINE_SIZE) + +/** Every hole is passed to `PolygonOptions.addHole` as a ring of its own, so each one has to hold up too. */ +internal fun PolygonDescriptor.isValid(): Boolean { + if (!coordinates.isValidPath(MINIMUM_RING_SIZE)) { + return false + } + + val rings = holes ?: return true + return rings.all { ring -> ring.isValidPath(MINIMUM_RING_SIZE) } +} + +/** `GoogleMap.addCircle` throws on a negative radius and on an unplaceable center. */ +internal fun CircleDescriptor.isValid(): Boolean = center.isValid() && radius.isFinite() && radius >= 0.0 + +private const val MINIMUM_POLYLINE_SIZE = 2 +private const val MINIMUM_RING_SIZE = 3 diff --git a/package/android/src/main/java/com/margelo/nitro/nitromaps/Region+Validity.kt b/package/android/src/main/java/com/margelo/nitro/nitromaps/Region+Validity.kt new file mode 100644 index 00000000..b66b3d3c --- /dev/null +++ b/package/android/src/main/java/com/margelo/nitro/nitromaps/Region+Validity.kt @@ -0,0 +1,9 @@ +package com.margelo.nitro.nitromaps + +/** A `NaN` or non-positive span puts the southern edge above the northern one, which `LatLngBounds` rejects. */ +internal fun Region.isValid(): Boolean = + isValidCoordinate(latitude, longitude) && + latitudeDelta.isFinite() && + latitudeDelta > 0.0 && + longitudeDelta.isFinite() && + longitudeDelta > 0.0 diff --git a/package/android/src/test/java/com/margelo/nitro/nitromaps/CoordinateValidityTest.kt b/package/android/src/test/java/com/margelo/nitro/nitromaps/CoordinateValidityTest.kt new file mode 100644 index 00000000..6cc0f16a --- /dev/null +++ b/package/android/src/test/java/com/margelo/nitro/nitromaps/CoordinateValidityTest.kt @@ -0,0 +1,51 @@ +package com.margelo.nitro.nitromaps + +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +class CoordinateValidityTest { + @Test + fun acceptsCoordinatesOnTheEdgeOfTheWorld() { + assertTrue(Coordinate(latitude = 90.0, longitude = 180.0).isValid()) + assertTrue(Coordinate(latitude = -90.0, longitude = -180.0).isValid()) + assertTrue(Coordinate(latitude = 52.23, longitude = 21.01).isValid()) + } + + @Test + fun rejectsNonFiniteCoordinates() { + assertFalse(Coordinate(latitude = Double.NaN, longitude = 0.0).isValid()) + assertFalse(Coordinate(latitude = 0.0, longitude = Double.NaN).isValid()) + assertFalse(Coordinate(latitude = Double.POSITIVE_INFINITY, longitude = 0.0).isValid()) + assertFalse(Coordinate(latitude = 0.0, longitude = Double.NEGATIVE_INFINITY).isValid()) + } + + @Test + fun rejectsCoordinatesOutsideTheWorld() { + assertFalse(Coordinate(latitude = 1000.0, longitude = 0.0).isValid()) + assertFalse(Coordinate(latitude = 90.0001, longitude = 0.0).isValid()) + assertFalse(Coordinate(latitude = -90.0001, longitude = 0.0).isValid()) + assertFalse(Coordinate(latitude = 0.0, longitude = 180.0001).isValid()) + assertFalse(Coordinate(latitude = 0.0, longitude = -180.0001).isValid()) + } + + @Test + fun requiresEnoughPointsForAPath() { + val point = Coordinate(latitude = 1.0, longitude = 2.0) + + assertFalse(emptyArray().isValidPath(minimumSize = 2)) + assertFalse(arrayOf(point).isValidPath(minimumSize = 2)) + assertTrue(arrayOf(point, point).isValidPath(minimumSize = 2)) + assertFalse(arrayOf(point, point).isValidPath(minimumSize = 3)) + assertTrue(arrayOf(point, point, point).isValidPath(minimumSize = 3)) + } + + @Test + fun rejectsAPathHoldingAnUnplaceablePoint() { + val point = Coordinate(latitude = 1.0, longitude = 2.0) + val broken = Coordinate(latitude = Double.NaN, longitude = 2.0) + + assertFalse(arrayOf(point, broken).isValidPath(minimumSize = 2)) + assertFalse(arrayOf(broken, point, point).isValidPath(minimumSize = 3)) + } +} diff --git a/package/android/src/test/java/com/margelo/nitro/nitromaps/OverlayDescriptorValidityTest.kt b/package/android/src/test/java/com/margelo/nitro/nitromaps/OverlayDescriptorValidityTest.kt new file mode 100644 index 00000000..6192749e --- /dev/null +++ b/package/android/src/test/java/com/margelo/nitro/nitromaps/OverlayDescriptorValidityTest.kt @@ -0,0 +1,86 @@ +package com.margelo.nitro.nitromaps + +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +class OverlayDescriptorValidityTest { + @Test + fun requiresTwoPlaceablePointsForAPolyline() { + assertFalse(polyline().isValid()) + assertFalse(polyline(point()).isValid()) + assertTrue(polyline(point(), point(latitude = 2.0)).isValid()) + assertFalse(polyline(point(), point(latitude = Double.NaN)).isValid()) + } + + @Test + fun requiresThreePlaceablePointsForAPolygonRing() { + assertFalse(polygon(point(), point()).isValid()) + assertTrue(polygon(point(), point(latitude = 2.0), point(longitude = 2.0)).isValid()) + } + + @Test + fun rejectsAPolygonWhoseHoleCannotBeDrawn() { + val ring = arrayOf(point(), point(latitude = 2.0), point(longitude = 2.0)) + + assertTrue(polygon(*ring, holes = arrayOf(ring)).isValid()) + assertFalse(polygon(*ring, holes = arrayOf(arrayOf(point(), point()))).isValid()) + assertFalse( + polygon(*ring, holes = arrayOf(arrayOf(point(), point(), point(latitude = Double.NaN)))) + .isValid(), + ) + } + + @Test + fun requiresANonNegativeFiniteRadiusForACircle() { + assertTrue(circle(radius = 0.0).isValid()) + assertTrue(circle(radius = 500.0).isValid()) + assertFalse(circle(radius = -1.0).isValid()) + assertFalse(circle(radius = Double.NaN).isValid()) + assertFalse(circle(center = point(latitude = Double.NaN)).isValid()) + } + + private fun point( + latitude: Double = 1.0, + longitude: Double = 1.0, + ): Coordinate = Coordinate(latitude = latitude, longitude = longitude) + + private fun polyline(vararg coordinates: Coordinate): PolylineDescriptor = + PolylineDescriptor( + id = "polyline-1", + coordinates = arrayOf(*coordinates), + strokeColor = null, + strokeWidth = null, + zIndex = null, + tappable = null, + ) + + private fun polygon( + vararg coordinates: Coordinate, + holes: Array>? = null, + ): PolygonDescriptor = + PolygonDescriptor( + id = "polygon-1", + coordinates = arrayOf(*coordinates), + holes = holes, + fillColor = null, + strokeColor = null, + strokeWidth = null, + zIndex = null, + tappable = null, + ) + + private fun circle( + center: Coordinate = point(), + radius: Double = 100.0, + ): CircleDescriptor = + CircleDescriptor( + id = "circle-1", + center = center, + radius = radius, + fillColor = null, + strokeColor = null, + strokeWidth = null, + tappable = null, + ) +} diff --git a/package/android/src/test/java/com/margelo/nitro/nitromaps/RegionValidityTest.kt b/package/android/src/test/java/com/margelo/nitro/nitromaps/RegionValidityTest.kt new file mode 100644 index 00000000..22313e41 --- /dev/null +++ b/package/android/src/test/java/com/margelo/nitro/nitromaps/RegionValidityTest.kt @@ -0,0 +1,50 @@ +package com.margelo.nitro.nitromaps + +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +class RegionValidityTest { + @Test + fun acceptsARegionLatLngBoundsCanRepresent() { + assertTrue(region().isValid()) + assertTrue( + region(latitude = 0.0, longitude = 180.0, latitudeDelta = 180.0, longitudeDelta = 360.0) + .isValid(), + ) + } + + @Test + fun rejectsANonFiniteRegion() { + assertFalse(region(latitude = Double.NaN, longitude = Double.NaN).isValid()) + assertFalse(region(latitudeDelta = Double.NaN).isValid()) + assertFalse(region(longitudeDelta = Double.POSITIVE_INFINITY).isValid()) + } + + @Test + fun rejectsACenterOutsideTheWorld() { + assertFalse(region(latitude = 1000.0).isValid()) + assertFalse(region(longitude = -180.0001).isValid()) + } + + @Test + fun rejectsASpanThatCoversNoArea() { + assertFalse(region(latitudeDelta = 0.0).isValid()) + assertFalse(region(longitudeDelta = 0.0).isValid()) + assertFalse(region(latitudeDelta = -0.1).isValid()) + assertFalse(region(longitudeDelta = -0.1).isValid()) + } + + private fun region( + latitude: Double = 52.23, + longitude: Double = 21.01, + latitudeDelta: Double = 0.1, + longitudeDelta: Double = 0.1, + ): Region = + Region( + latitude = latitude, + longitude = longitude, + latitudeDelta = latitudeDelta, + longitudeDelta = longitudeDelta, + ) +} diff --git a/package/ios/AppleMapProviderAdapter.swift b/package/ios/AppleMapProviderAdapter.swift index fb73d5b0..e1a667ba 100644 --- a/package/ios/AppleMapProviderAdapter.swift +++ b/package/ios/AppleMapProviderAdapter.swift @@ -217,12 +217,13 @@ final class AppleMapProviderAdapter: MapProviderAdapter { padding: EdgePadding?, animated: Bool? ) throws { - guard !coordinates.isEmpty else { + let validCoordinates = coordinates.filter { $0.isValid } + guard !validCoordinates.isEmpty else { return } var mapRect = MKMapRect.null - for coordinate in coordinates { + for coordinate in validCoordinates { let mapPoint = MKMapPoint( CLLocationCoordinate2D( latitude: coordinate.latitude, @@ -243,7 +244,16 @@ final class AppleMapProviderAdapter: MapProviderAdapter { } func applyRegion(_ region: Region, animated: Bool = false) { - let targetRegion = region.toMKCoordinateRegion() + // `setRegion` raises an NSException Swift cannot catch, so there is no + // recovery once an invalid region has been handed over. `regionThatFits` + // pulls a span whose edges run past a pole back to something MapKit can + // show, as `animateToClusterRegion` already does; the guard covers what it + // cannot fix - a non-finite or out-of-range center. + guard region.isValid else { + return + } + + let targetRegion = view.regionThatFits(region.toMKCoordinateRegion()) guard !view.region.approximatelyEquals(targetRegion) else { return } diff --git a/package/ios/Coordinate+Validity.swift b/package/ios/Coordinate+Validity.swift new file mode 100644 index 00000000..5edd1628 --- /dev/null +++ b/package/ios/Coordinate+Validity.swift @@ -0,0 +1,7 @@ +import CoreLocation + +extension Coordinate { + var isValid: Bool { + CLLocationCoordinate2DIsValid(toCLLocationCoordinate2D()) + } +} diff --git a/package/ios/Geometry/RegionSpan.swift b/package/ios/Geometry/RegionSpan.swift new file mode 100644 index 00000000..3ec6741d --- /dev/null +++ b/package/ios/Geometry/RegionSpan.swift @@ -0,0 +1,14 @@ +import Foundation + +/// The span half of a region, which `CLLocationCoordinate2DIsValid` says nothing about. +enum RegionSpan { + /// A span has to cover an area: a `NaN` or non-positive delta collapses the + /// region, and on Android the halved deltas would put the southern edge above + /// the northern one, which `LatLngBounds` rejects. + static func isDrawable(latitudeDelta: Double, longitudeDelta: Double) -> Bool { + latitudeDelta.isFinite + && latitudeDelta > 0 + && longitudeDelta.isFinite + && longitudeDelta > 0 + } +} diff --git a/package/ios/GoogleMapProviderAdapter.swift b/package/ios/GoogleMapProviderAdapter.swift index 591ad83d..8abf4612 100644 --- a/package/ios/GoogleMapProviderAdapter.swift +++ b/package/ios/GoogleMapProviderAdapter.swift @@ -246,12 +246,13 @@ final class GoogleMapProviderAdapter: NSObject, MapProviderAdapter { padding: EdgePadding?, animated: Bool? ) throws { - guard !coordinates.isEmpty else { + let validCoordinates = coordinates.filter { $0.isValid } + guard !validCoordinates.isEmpty else { return } var bounds = GMSCoordinateBounds() - for coordinate in coordinates { + for coordinate in validCoordinates { bounds = bounds.includingCoordinate(coordinate.toCLLocationCoordinate2D()) } let edgePadding = padding?.toUIEdgeInsets() ?? .zero @@ -303,6 +304,10 @@ final class GoogleMapProviderAdapter: NSObject, MapProviderAdapter { } private func applyRegion(_ region: Region, animated: Bool = false) { + guard region.isValid else { + return + } + applyCameraUpdate( GMSCameraUpdate.fit(region.toGMSCoordinateBounds(), with: mapPadding?.toUIEdgeInsets() ?? .zero), animated: animated, diff --git a/package/ios/Package.swift b/package/ios/Package.swift index bb5275c9..c4378bf4 100644 --- a/package/ios/Package.swift +++ b/package/ios/Package.swift @@ -2,18 +2,29 @@ import PackageDescription +// Holds the parts of package/ios that need no MapKit, UIKit or Nitro-generated +// types, so `swift test` can cover them. The podspec compiles them as well. let package = Package( - name: "NitroMapsColorParser", + name: "NitroMapsSupport", platforms: [.macOS(.v13)], targets: [ .target( name: "NitroMapsColorParser", path: "ColorParser" ), + .target( + name: "NitroMapsGeometry", + path: "Geometry" + ), .testTarget( name: "NitroMapsColorParserTests", dependencies: ["NitroMapsColorParser"], - path: "Tests" + path: "Tests/ColorParser" + ), + .testTarget( + name: "NitroMapsGeometryTests", + dependencies: ["NitroMapsGeometry"], + path: "Tests/Geometry" ), ] ) diff --git a/package/ios/Region+GMSCoordinateBounds.swift b/package/ios/Region+GMSCoordinateBounds.swift index c4d61adf..4c4e41fb 100644 --- a/package/ios/Region+GMSCoordinateBounds.swift +++ b/package/ios/Region+GMSCoordinateBounds.swift @@ -4,12 +4,15 @@ import MapKit extension Region { func toGMSCoordinateBounds() -> GMSCoordinateBounds { + // Latitude is clamped rather than wrapped: a span wide enough to run past a + // pole has no representable edge there, and Google Maps is handed the + // closest one that does exist. let southWest = CLLocationCoordinate2D( - latitude: latitude - latitudeDelta / 2, + latitude: max(-90, latitude - latitudeDelta / 2), longitude: longitude - longitudeDelta / 2 ) let northEast = CLLocationCoordinate2D( - latitude: latitude + latitudeDelta / 2, + latitude: min(90, latitude + latitudeDelta / 2), longitude: longitude + longitudeDelta / 2 ) return GMSCoordinateBounds(coordinate: southWest, coordinate: northEast) diff --git a/package/ios/Region+Validity.swift b/package/ios/Region+Validity.swift new file mode 100644 index 00000000..74a5adee --- /dev/null +++ b/package/ios/Region+Validity.swift @@ -0,0 +1,9 @@ +import CoreLocation + +extension Region { + var isValid: Bool { + CLLocationCoordinate2DIsValid( + CLLocationCoordinate2D(latitude: latitude, longitude: longitude) + ) && RegionSpan.isDrawable(latitudeDelta: latitudeDelta, longitudeDelta: longitudeDelta) + } +} diff --git a/package/ios/Tests/HexColorComponentsTests.swift b/package/ios/Tests/ColorParser/HexColorComponentsTests.swift similarity index 100% rename from package/ios/Tests/HexColorComponentsTests.swift rename to package/ios/Tests/ColorParser/HexColorComponentsTests.swift diff --git a/package/ios/Tests/Geometry/RegionSpanTests.swift b/package/ios/Tests/Geometry/RegionSpanTests.swift new file mode 100644 index 00000000..5cea07d2 --- /dev/null +++ b/package/ios/Tests/Geometry/RegionSpanTests.swift @@ -0,0 +1,18 @@ +import Testing + +@testable import NitroMapsGeometry + +@Test +func acceptsASpanThatCoversAnArea() { + #expect(RegionSpan.isDrawable(latitudeDelta: 0.1, longitudeDelta: 0.1)) + #expect(RegionSpan.isDrawable(latitudeDelta: 180, longitudeDelta: 360)) +} + +@Test +func rejectsASpanThatCoversNoArea() { + #expect(!RegionSpan.isDrawable(latitudeDelta: 0, longitudeDelta: 0.1)) + #expect(!RegionSpan.isDrawable(latitudeDelta: 0.1, longitudeDelta: 0)) + #expect(!RegionSpan.isDrawable(latitudeDelta: -0.1, longitudeDelta: 0.1)) + #expect(!RegionSpan.isDrawable(latitudeDelta: .nan, longitudeDelta: 0.1)) + #expect(!RegionSpan.isDrawable(latitudeDelta: 0.1, longitudeDelta: .infinity)) +} diff --git a/package/src/components/MapView.tsx b/package/src/components/MapView.tsx index 132b08ed..73dc78c8 100644 --- a/package/src/components/MapView.tsx +++ b/package/src/components/MapView.tsx @@ -24,6 +24,8 @@ import { import { OverlayType, overlayCallbackKey } from '../overlays/overlayType'; import { normalizeMarkerDescriptors } from '../overlays/normalizeMarkerDescriptors'; import { resolveMapProvider } from '../providers'; +import { resolveFitCoordinates } from '../region/resolveFitCoordinates'; +import { useValidRegion } from '../region/useValidRegion'; import type { Coordinate } from '../types/coordinate'; import type { MapViewProps, PoiPressEvent } from '../types/map'; import type { MapViewRef } from '../types/ref'; @@ -130,6 +132,7 @@ export function MapView({ normalizeEnteringAnimation(clusterEnteringAnimation), enteringAnimationsEqual, ); + const validRegion = useValidRegion(region); const hasMarkerPress = onMarkerPressProp != null || hasCollectedMarkerPress; @@ -262,7 +265,11 @@ export function MapView({ withHybridRef(hybridRef, (hybrid) => hybrid.getVisibleRegion()), fitToCoordinates: (coordinates, padding, animated) => withHybridRef(hybridRef, (hybrid) => - hybrid.fitToCoordinates(coordinates, padding, animated), + hybrid.fitToCoordinates( + resolveFitCoordinates(coordinates), + padding, + animated, + ), ), }), [], @@ -276,7 +283,7 @@ export function MapView({ provider={resolvedProvider} googleMapId={googleMapId} mapType={mapType} - region={region} + region={validRegion} camera={camera} scrollEnabled={scrollEnabled} zoomEnabled={zoomEnabled} diff --git a/package/src/geojson/__tests__/warnGeojson.test.ts b/package/src/geojson/__tests__/warnGeojson.test.ts index d38f0417..f7efc1f7 100644 --- a/package/src/geojson/__tests__/warnGeojson.test.ts +++ b/package/src/geojson/__tests__/warnGeojson.test.ts @@ -1,4 +1,4 @@ -import { afterEach, describe, expect, spyOn, test } from 'bun:test'; +import { afterEach, beforeEach, describe, expect, spyOn, test } from 'bun:test'; import { warnGeojson } from '../warnGeojson'; const warnSpy = spyOn(console, 'warn'); @@ -14,6 +14,10 @@ function restoreDevFlag(): void { globalDev.__DEV__ = previousDev; } +beforeEach(() => { + warnSpy.mockClear(); +}); + afterEach(() => { warnSpy.mockClear(); restoreDevFlag(); diff --git a/package/src/geojson/warnGeojson.ts b/package/src/geojson/warnGeojson.ts index bfc13a8d..bf88f2cc 100644 --- a/package/src/geojson/warnGeojson.ts +++ b/package/src/geojson/warnGeojson.ts @@ -1,7 +1,3 @@ -export function warnGeojson(message: string): void { - if ((globalThis as { __DEV__?: boolean }).__DEV__ !== true) { - return; - } +import { createWarn } from '../utils/warn'; - console.warn(`[react-native-better-maps] Geojson: ${message}`); -} +export const warnGeojson = createWarn('Geojson'); diff --git a/package/src/overlays/__tests__/warnOverlay.test.ts b/package/src/overlays/__tests__/warnOverlay.test.ts index 1170ca92..46ab754f 100644 --- a/package/src/overlays/__tests__/warnOverlay.test.ts +++ b/package/src/overlays/__tests__/warnOverlay.test.ts @@ -1,4 +1,4 @@ -import { afterEach, describe, expect, spyOn, test } from 'bun:test'; +import { afterEach, beforeEach, describe, expect, spyOn, test } from 'bun:test'; import { warnOverlay } from '../warnOverlay'; const warnSpy = spyOn(console, 'warn'); @@ -14,6 +14,10 @@ function restoreDevFlag(): void { globalDev.__DEV__ = previousDev; } +beforeEach(() => { + warnSpy.mockClear(); +}); + afterEach(() => { warnSpy.mockClear(); restoreDevFlag(); diff --git a/package/src/overlays/collectOverlayChild.ts b/package/src/overlays/collectOverlayChild.ts index b501b511..51e3ecc2 100644 --- a/package/src/overlays/collectOverlayChild.ts +++ b/package/src/overlays/collectOverlayChild.ts @@ -23,7 +23,7 @@ import { isValidCoordinate, isValidCoordinateList, isValidRadius, -} from './validateOverlay'; +} from '../utils/validateGeometry'; import { warnOverlay } from './warnOverlay'; import { collectMarkerOverlay, diff --git a/package/src/overlays/warnOverlay.ts b/package/src/overlays/warnOverlay.ts index 76c03184..c2b18fc7 100644 --- a/package/src/overlays/warnOverlay.ts +++ b/package/src/overlays/warnOverlay.ts @@ -1,7 +1,3 @@ -export function warnOverlay(message: string): void { - if ((globalThis as { __DEV__?: boolean }).__DEV__ !== true) { - return; - } +import { createWarn } from '../utils/warn'; - console.warn(`[react-native-better-maps] Overlay: ${message}`); -} +export const warnOverlay = createWarn('Overlay'); diff --git a/package/src/region/__tests__/isValidRegion.test.ts b/package/src/region/__tests__/isValidRegion.test.ts new file mode 100644 index 00000000..bd6f5bbc --- /dev/null +++ b/package/src/region/__tests__/isValidRegion.test.ts @@ -0,0 +1,54 @@ +import { describe, expect, test } from 'bun:test'; +import { isValidRegion } from '../isValidRegion'; + +const validRegion = { + latitude: 52.23, + longitude: 21.01, + latitudeDelta: 0.1, + longitudeDelta: 0.1, +}; + +describe('isValidRegion', () => { + test('accepts a region both SDKs can represent', () => { + expect(isValidRegion(validRegion)).toBe(true); + expect( + isValidRegion({ + latitude: 0, + longitude: 180, + latitudeDelta: 180, + longitudeDelta: 360, + }), + ).toBe(true); + }); + + test('rejects a missing region', () => { + expect(isValidRegion(undefined)).toBe(false); + }); + + test('rejects a non-finite center', () => { + expect(isValidRegion({ ...validRegion, latitude: Number.NaN })).toBe(false); + expect( + isValidRegion({ ...validRegion, longitude: Number.POSITIVE_INFINITY }), + ).toBe(false); + }); + + test('rejects a center outside the world', () => { + expect(isValidRegion({ ...validRegion, latitude: 1000 })).toBe(false); + expect(isValidRegion({ ...validRegion, latitude: -90.0001 })).toBe(false); + expect(isValidRegion({ ...validRegion, longitude: 180.0001 })).toBe(false); + }); + + test('rejects deltas that do not span an area', () => { + expect(isValidRegion({ ...validRegion, latitudeDelta: 0 })).toBe(false); + expect(isValidRegion({ ...validRegion, longitudeDelta: -1 })).toBe(false); + expect(isValidRegion({ ...validRegion, latitudeDelta: Number.NaN })).toBe( + false, + ); + expect( + isValidRegion({ + ...validRegion, + longitudeDelta: Number.POSITIVE_INFINITY, + }), + ).toBe(false); + }); +}); diff --git a/package/src/region/__tests__/resolveFitCoordinates.test.ts b/package/src/region/__tests__/resolveFitCoordinates.test.ts new file mode 100644 index 00000000..ce38f83e --- /dev/null +++ b/package/src/region/__tests__/resolveFitCoordinates.test.ts @@ -0,0 +1,58 @@ +import { afterEach, beforeEach, describe, expect, spyOn, test } from 'bun:test'; +import { resolveFitCoordinates } from '../resolveFitCoordinates'; + +const warnSpy = spyOn(console, 'warn'); +const previousDev = (globalThis as { __DEV__?: boolean }).__DEV__; + +function restoreDevFlag(): void { + const globalDev = globalThis as { __DEV__?: boolean }; + if (previousDev === undefined) { + delete globalDev.__DEV__; + return; + } + + globalDev.__DEV__ = previousDev; +} + +beforeEach(() => { + warnSpy.mockClear(); + (globalThis as { __DEV__?: boolean }).__DEV__ = true; +}); + +afterEach(() => { + warnSpy.mockClear(); + restoreDevFlag(); +}); + +describe('resolveFitCoordinates', () => { + test('passes placeable coordinates through without warning', () => { + const coordinates = [ + { latitude: 52.23, longitude: 21.01 }, + { latitude: 50.06, longitude: 19.94 }, + ]; + + expect(resolveFitCoordinates(coordinates)).toEqual(coordinates); + expect(warnSpy).not.toHaveBeenCalled(); + }); + + test('keeps the placeable coordinates and warns about the rest', () => { + const good = { latitude: 52.23, longitude: 21.01 }; + + const result = resolveFitCoordinates([ + good, + { latitude: Number.NaN, longitude: 21.01 }, + { latitude: 1000, longitude: 0 }, + ]); + + expect(result).toEqual([good]); + expect(warnSpy).toHaveBeenCalledTimes(1); + expect(warnSpy.mock.calls[0]?.[0]).toContain('2 coordinate(s)'); + }); + + test('returns nothing when no coordinate can be placed', () => { + expect( + resolveFitCoordinates([{ latitude: Number.NaN, longitude: Number.NaN }]), + ).toEqual([]); + expect(warnSpy).toHaveBeenCalledTimes(1); + }); +}); diff --git a/package/src/region/__tests__/resolveRegionProp.test.ts b/package/src/region/__tests__/resolveRegionProp.test.ts new file mode 100644 index 00000000..71b20b81 --- /dev/null +++ b/package/src/region/__tests__/resolveRegionProp.test.ts @@ -0,0 +1,77 @@ +import { afterEach, beforeEach, describe, expect, spyOn, test } from 'bun:test'; +import { resolveRegionProp } from '../resolveRegionProp'; + +const warnSpy = spyOn(console, 'warn'); +const previousDev = (globalThis as { __DEV__?: boolean }).__DEV__; + +const validRegion = { + latitude: 52.23, + longitude: 21.01, + latitudeDelta: 0.1, + longitudeDelta: 0.1, +}; + +function restoreDevFlag(): void { + const globalDev = globalThis as { __DEV__?: boolean }; + if (previousDev === undefined) { + delete globalDev.__DEV__; + return; + } + + globalDev.__DEV__ = previousDev; +} + +beforeEach(() => { + warnSpy.mockClear(); +}); + +afterEach(() => { + warnSpy.mockClear(); + restoreDevFlag(); +}); + +describe('resolveRegionProp', () => { + test('passes a valid region through unchanged', () => { + (globalThis as { __DEV__?: boolean }).__DEV__ = true; + + expect(resolveRegionProp(validRegion, undefined)).toBe(validRegion); + expect(warnSpy).not.toHaveBeenCalled(); + }); + + test('leaves a missing region alone without warning', () => { + (globalThis as { __DEV__?: boolean }).__DEV__ = true; + + expect(resolveRegionProp(undefined, undefined)).toBeUndefined(); + expect(warnSpy).not.toHaveBeenCalled(); + }); + + test('drops an invalid region and warns in development', () => { + (globalThis as { __DEV__?: boolean }).__DEV__ = true; + + expect( + resolveRegionProp({ ...validRegion, latitude: Number.NaN }, undefined), + ).toBeUndefined(); + + expect(warnSpy).toHaveBeenCalledTimes(1); + expect(warnSpy.mock.calls[0]?.[0]).toContain('region ignored'); + }); + + test('holds the last accepted region instead of unsetting the prop', () => { + (globalThis as { __DEV__?: boolean }).__DEV__ = true; + + // Unsetting it would reach the native view as `null`, which the generated + // struct converter rejects before any native guard runs. + expect( + resolveRegionProp({ ...validRegion, latitude: Number.NaN }, validRegion), + ).toBe(validRegion); + }); + + test('drops an invalid region silently outside development', () => { + (globalThis as { __DEV__?: boolean }).__DEV__ = false; + + expect( + resolveRegionProp({ ...validRegion, latitude: 1000 }, undefined), + ).toBeUndefined(); + expect(warnSpy).not.toHaveBeenCalled(); + }); +}); diff --git a/package/src/region/isValidRegion.ts b/package/src/region/isValidRegion.ts new file mode 100644 index 00000000..a9a4b578 --- /dev/null +++ b/package/src/region/isValidRegion.ts @@ -0,0 +1,16 @@ +import type { Region } from '../types/region'; +import { isValidCoordinate } from '../utils/validateGeometry'; + +export function isValidRegion(value: Region | undefined): boolean { + return ( + value != null && + isValidCoordinate({ + latitude: value.latitude, + longitude: value.longitude, + }) && + Number.isFinite(value.latitudeDelta) && + value.latitudeDelta > 0 && + Number.isFinite(value.longitudeDelta) && + value.longitudeDelta > 0 + ); +} diff --git a/package/src/region/resolveFitCoordinates.ts b/package/src/region/resolveFitCoordinates.ts new file mode 100644 index 00000000..6d58d418 --- /dev/null +++ b/package/src/region/resolveFitCoordinates.ts @@ -0,0 +1,20 @@ +import type { Coordinate } from '../types/coordinate'; +import { isValidCoordinate } from '../utils/validateGeometry'; +import { warnRegion } from './warnRegion'; + +// The native adapters drop unplaceable coordinates too - they have to, since +// `hybridRef` reaches them directly - but only Android can report it, and to +// logcat. Filtering here is what gives both platforms the same warning. +export function resolveFitCoordinates(coordinates: Coordinate[]): Coordinate[] { + const placeable = coordinates.filter((coordinate) => + isValidCoordinate(coordinate), + ); + + if (placeable.length !== coordinates.length) { + warnRegion( + `fitToCoordinates ignored ${coordinates.length - placeable.length} coordinate(s) outside the world`, + ); + } + + return placeable; +} diff --git a/package/src/region/resolveRegionProp.ts b/package/src/region/resolveRegionProp.ts new file mode 100644 index 00000000..aadcf6dc --- /dev/null +++ b/package/src/region/resolveRegionProp.ts @@ -0,0 +1,22 @@ +import type { Region } from '../types/region'; +import { isValidRegion } from './isValidRegion'; +import { warnRegion } from './warnRegion'; + +// The native prop must never go from a region back to `undefined`: React +// rewrites a removed prop to `null` (ReactNativeAttributePayload), the optional +// JSI converter only short-circuits on `undefined`, and the generated struct +// converter then calls `asObject` on a null and throws - before any native +// guard runs. Holding the last accepted region leaves the prop untouched +// instead, which is also what the map should show. +export function resolveRegionProp( + region: Region | undefined, + lastAccepted: Region | undefined, +): Region | undefined { + if (region == null || isValidRegion(region)) { + return region; + } + + warnRegion('region ignored: invalid center coordinate or deltas'); + + return lastAccepted; +} diff --git a/package/src/region/useValidRegion.ts b/package/src/region/useValidRegion.ts new file mode 100644 index 00000000..449e21e4 --- /dev/null +++ b/package/src/region/useValidRegion.ts @@ -0,0 +1,28 @@ +import { useLayoutEffect, useMemo, useRef } from 'react'; +import type { Region } from '../types/region'; +import { resolveRegionProp } from './resolveRegionProp'; + +/** + * Holds the last region the native view accepted, so an invalid one can be + * dropped without the prop ever transitioning back to `undefined`. + * + * Memoized on region identity so a stable object is checked - and warned about + * - once rather than on every render. The ref is written after commit, as in + * `useStableValue`: a value remembered from a render React discarded is a value + * the native view never received. + */ +export function useValidRegion(region: Region | undefined): Region | undefined { + const lastAccepted = useRef(undefined); + // Reading the ref here is safe: it only matters at the moment `region` + // changes, which is exactly when this recomputes. + const accepted = useMemo( + () => resolveRegionProp(region, lastAccepted.current), + [region], + ); + + useLayoutEffect(() => { + lastAccepted.current = accepted; + }, [accepted]); + + return accepted; +} diff --git a/package/src/region/warnRegion.ts b/package/src/region/warnRegion.ts new file mode 100644 index 00000000..601bd570 --- /dev/null +++ b/package/src/region/warnRegion.ts @@ -0,0 +1,3 @@ +import { createWarn } from '../utils/warn'; + +export const warnRegion = createWarn('Region'); diff --git a/package/src/overlays/__tests__/validateOverlay.test.ts b/package/src/utils/__tests__/validateGeometry.test.ts similarity index 96% rename from package/src/overlays/__tests__/validateOverlay.test.ts rename to package/src/utils/__tests__/validateGeometry.test.ts index 5fcc625f..f766a97d 100644 --- a/package/src/overlays/__tests__/validateOverlay.test.ts +++ b/package/src/utils/__tests__/validateGeometry.test.ts @@ -3,9 +3,9 @@ import { isValidCoordinate, isValidCoordinateList, isValidRadius, -} from '../validateOverlay'; +} from '../validateGeometry'; -describe('overlay validation', () => { +describe('geometry validation', () => { test('validates coordinate bounds and finite values', () => { expect(isValidCoordinate({ latitude: 90, longitude: 180 })).toBe(true); expect(isValidCoordinate({ latitude: -90, longitude: -180 })).toBe(true); diff --git a/package/src/overlays/validateOverlay.ts b/package/src/utils/validateGeometry.ts similarity index 100% rename from package/src/overlays/validateOverlay.ts rename to package/src/utils/validateGeometry.ts diff --git a/package/src/utils/warn.ts b/package/src/utils/warn.ts new file mode 100644 index 00000000..0cf573af --- /dev/null +++ b/package/src/utils/warn.ts @@ -0,0 +1,9 @@ +export function createWarn(scope: string): (message: string) => void { + return (message: string) => { + if ((globalThis as { __DEV__?: boolean }).__DEV__ !== true) { + return; + } + + console.warn(`[react-native-better-maps] ${scope}: ${message}`); + }; +} From 3f0ec404a6e01a8ab4e91b4bb3babbad2259c16a Mon Sep 17 00:00:00 2001 From: Jakub Kasprzyk Date: Mon, 21 Sep 2026 13:18:13 +0200 Subject: [PATCH 2/2] fix: hold the last region when the prop is unset Unsetting `region` on a mounted view reached the native prop as `null` - React rewrites a removed prop that way - and the generated struct converter throws on it, the same crash an invalid region used to cause. The invariant the file already documented now covers both transitions. --- .../__tests__/resolveRegionProp.test.ts | 9 +++++++++ package/src/region/resolveRegionProp.ts | 20 ++++++++++++------- package/src/region/useValidRegion.ts | 4 ++-- 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/package/src/region/__tests__/resolveRegionProp.test.ts b/package/src/region/__tests__/resolveRegionProp.test.ts index 71b20b81..11151501 100644 --- a/package/src/region/__tests__/resolveRegionProp.test.ts +++ b/package/src/region/__tests__/resolveRegionProp.test.ts @@ -45,6 +45,15 @@ describe('resolveRegionProp', () => { expect(warnSpy).not.toHaveBeenCalled(); }); + test('holds the last accepted region when the prop is unset', () => { + (globalThis as { __DEV__?: boolean }).__DEV__ = true; + + // `region={enabled ? region : undefined}` on a mounted view: unsetting it + // reaches the native view as `null`, which throws in the struct converter. + expect(resolveRegionProp(undefined, validRegion)).toBe(validRegion); + expect(warnSpy).not.toHaveBeenCalled(); + }); + test('drops an invalid region and warns in development', () => { (globalThis as { __DEV__?: boolean }).__DEV__ = true; diff --git a/package/src/region/resolveRegionProp.ts b/package/src/region/resolveRegionProp.ts index aadcf6dc..d989309c 100644 --- a/package/src/region/resolveRegionProp.ts +++ b/package/src/region/resolveRegionProp.ts @@ -2,17 +2,23 @@ import type { Region } from '../types/region'; import { isValidRegion } from './isValidRegion'; import { warnRegion } from './warnRegion'; -// The native prop must never go from a region back to `undefined`: React -// rewrites a removed prop to `null` (ReactNativeAttributePayload), the optional -// JSI converter only short-circuits on `undefined`, and the generated struct -// converter then calls `asObject` on a null and throws - before any native -// guard runs. Holding the last accepted region leaves the prop untouched -// instead, which is also what the map should show. +// Once the view has accepted a region the prop must never go back to +// `undefined`, whether it was unset or is unusable: React rewrites a removed +// prop to `null` (ReactNativeAttributePayload), the optional JSI converter only +// short-circuits on `undefined`, and the generated struct converter then calls +// `asObject` on a null and throws - before any native guard runs. Holding the +// last accepted region leaves the prop untouched instead, which is also what +// the map should show. Before anything has been accepted there is nothing to +// hold, and `undefined` at mount is safe: React omits the key entirely. export function resolveRegionProp( region: Region | undefined, lastAccepted: Region | undefined, ): Region | undefined { - if (region == null || isValidRegion(region)) { + if (region == null) { + return lastAccepted; + } + + if (isValidRegion(region)) { return region; } diff --git a/package/src/region/useValidRegion.ts b/package/src/region/useValidRegion.ts index 449e21e4..a37dc1e7 100644 --- a/package/src/region/useValidRegion.ts +++ b/package/src/region/useValidRegion.ts @@ -3,8 +3,8 @@ import type { Region } from '../types/region'; import { resolveRegionProp } from './resolveRegionProp'; /** - * Holds the last region the native view accepted, so an invalid one can be - * dropped without the prop ever transitioning back to `undefined`. + * Holds the last region the native view accepted, so an unset or invalid one + * never makes the prop transition back to `undefined`. * * Memoized on region identity so a stable object is checked - and warned about * - once rather than on every render. The ref is written after commit, as in