Skip to content

Commit e921ea3

Browse files
mfazekasclaude
andauthored
refactor(iOS): null safety for mapView addToMap, removeFromMap (#4079)
* refactor(ios): introduce RNMBXMapAndMapViewComponent protocol for type-safe mapView access This commit addresses the issue raised in PR #3963 regarding the optional mapView property access pattern that could lead to nil access crashes. Key Changes: - Created new RNMBXMapAndMapViewComponent protocol that guarantees non-nil MapView parameter in addToMap and removeFromMap methods - Kept existing RNMBXMapComponent protocol for components that don't require direct MapView access - Updated RNMBXMapView to handle both protocol types and validate mapView presence before calling RNMBXMapAndMapViewComponent methods - Created RNMBXMapAndMapViewComponentBase as a base class for components requiring MapView Components migrated to RNMBXMapAndMapViewComponent: - RNMBXCustomLocationProvider: Needs mapView for location provider setup - RNMBXCamera: Accesses mapView.viewport for status observers - RNMBXViewport: Directly manages viewport state - RNMBXNativeUserLocation: Configures location puck via mapView.location - RNMBXInteractiveElement: Base class that accesses mapView.mapboxMap.style - RNMBXSource: Manages sources via mapView.mapboxMap.style - RNMBXPointAnnotation: Inherits from RNMBXInteractiveElement Architecture Benefits: - Type system enforces mapView availability at compile time - Clear contract: components explicitly declare mapView requirement - Eliminates defensive nil checks in component implementations - Centralizes nil checking in RNMBXMapView with proper error logging - Maintains invariant: if addedToMap is true, mapView must be non-nil This approach is superior to PR #3963's defensive nil checks because: 1. It makes the requirement explicit via protocol 2. Failures are logged at the framework level, not silently ignored 3. Type safety prevents accidental nil access 4. Components can safely assume mapView is valid in lifecycle methods Related: #3963 * refactor(ios): improve protocol hierarchy with inheritance and default implementations Refinements to the RNMBXMapComponent protocol architecture: 1. Extracted common protocol requirement into base protocol: - Created RNMBXMapComponentProtocol with waitForStyleLoad() method - Both RNMBXMapComponent and RNMBXMapAndMapViewComponent inherit from it - Eliminates duplication of waitForStyleLoad() in both protocols 2. Added default implementation via Swift protocol extension: - RNMBXMapComponentProtocol extension provides default waitForStyleLoad() -> false - Components that need style load can override, others use default - Removed redundant implementations from: * RNMBXMapComponentBase * RNMBXMapAndMapViewComponentBase * RNMBXCustomLocationProvider * RNMBXImages Benefits: - DRY: Single definition of waitForStyleLoad() requirement - Convenience: Default implementation reduces boilerplate - Clarity: Explicit protocol inheritance shows relationship - Type safety: Compiler enforces common interface This follows Swift best practices for protocol-oriented programming. * refactor(ios): make RNMBXMapAndMapViewComponent inherit from RNMBXMapComponent Changed RNMBXMapAndMapViewComponent to inherit from RNMBXMapComponent instead of RNMBXMapComponentProtocol, establishing a proper subtype relationship. Key Changes: 1. Protocol Inheritance: - RNMBXMapAndMapViewComponent now extends RNMBXMapComponent - This makes semantic sense: "a component that needs MapView IS A map component" - Ensures compatibility with existing code that checks for RNMBXMapComponent 2. Default Implementation Safety Guard: - Added protocol extension with default implementations of base protocol methods - These implementations log errors and trigger assertions if called - Forces developers to use the mapView-aware versions - Catches mistakes at runtime (or compile time in debug builds) 3. Updated RNMBXSource to Handle Both Protocols: - Changed components array type to RNMBXMapComponentProtocol (base protocol) - Check for more specific protocol first (RNMBXMapAndMapViewComponent) - Then fallback to RNMBXMapComponent for components that don't need mapView - Applied this pattern to insertReactSubviewInternal, removeReactSubviewInternal, addToMap, and removeFromMap methods Benefits: - Type system correctly models the relationship (MapAndMapViewComponent IS A MapComponent) - Existing code that checks "as? RNMBXMapComponent" still works (matches both types) - Safety: Can't accidentally call wrong method signature due to default implementations - Future-proof: New code can store both types in arrays typed as RNMBXMapComponent RNMBXMapView already checks for the more specific protocol first, so no changes needed there - the inheritance makes the code more correct without breaking anything. * ios: build fix * Update ios/RNMBX/RNMBXCamera.swift * Update ios/RNMBX/RNMBXCamera.swift * Update ios/RNMBX/RNMBXCamera.swift --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 8d181e8 commit e921ea3

10 files changed

+278
-118
lines changed

ios/RNMBX/RNMBXCamera.swift

Lines changed: 88 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,46 @@ public enum RemovalReason {
1515
case ViewRemoval, StyleChange, OnDestroy, ComponentChange, Reorder
1616
}
1717

18-
public protocol RNMBXMapComponent: AnyObject {
18+
/// Base protocol for all map components
19+
public protocol RNMBXMapComponentProtocol: AnyObject {
20+
func waitForStyleLoad() -> Bool
21+
}
22+
23+
/// Default implementation: most components don't need to wait for style load
24+
extension RNMBXMapComponentProtocol {
25+
public func waitForStyleLoad() -> Bool {
26+
return false
27+
}
28+
}
29+
30+
/// Protocol for components that can work without direct MapView access
31+
public protocol RNMBXMapComponent: RNMBXMapComponentProtocol {
1932
func addToMap(_ map: RNMBXMapView, style: Style)
2033
func removeFromMap(_ map: RNMBXMapView, reason: RemovalReason) -> Bool
21-
22-
func waitForStyleLoad() -> Bool
34+
}
35+
36+
/// Protocol for components that require a valid MapView instance for both add and remove operations.
37+
/// Use this protocol when your component needs to interact with the native MapView directly.
38+
/// The MapView parameter is guaranteed to be non-nil when these methods are called.
39+
///
40+
/// This protocol inherits from RNMBXMapComponent to ensure compatibility with existing code,
41+
/// but provides default implementations of the base protocol methods that throw errors,
42+
/// forcing implementers to use the mapView-aware versions.
43+
public protocol RNMBXMapAndMapViewComponent: RNMBXMapComponent {
44+
func addToMap(_ map: RNMBXMapView, mapView: MapView, style: Style)
45+
func removeFromMap(_ map: RNMBXMapView, mapView: MapView, reason: RemovalReason) -> Bool
46+
}
47+
48+
/// Default implementations for RNMBXMapAndMapViewComponent that prevent accidental use of base protocol methods
49+
extension RNMBXMapAndMapViewComponent {
50+
public func addToMap(_ map: RNMBXMapView, style: Style) {
51+
Logger.error("CRITICAL: addToMap(_:style:) called on RNMBXMapAndMapViewComponent. Use addToMap(_:mapView:style:) instead. Component: \(type(of: self))")
52+
}
53+
54+
public func removeFromMap(_ map: RNMBXMapView, reason: RemovalReason) -> Bool {
55+
Logger.error("CRITICAL: removeFromMap(_:reason:) called on RNMBXMapAndMapViewComponent. Use removeFromMap(_:mapView:reason:) instead. Component: \(type(of: self))")
56+
return false
57+
}
2358
}
2459

2560
enum CameraMode: Int {
@@ -85,7 +120,7 @@ class CameraUpdateQueue {
85120
open class RNMBXMapComponentBase : UIView, RNMBXMapComponent {
86121
private weak var _map: RNMBXMapView! = nil
87122
private var _mapCallbacks: [(RNMBXMapView) -> Void] = []
88-
123+
89124
weak var map : RNMBXMapView? {
90125
return _map;
91126
}
@@ -103,28 +138,64 @@ open class RNMBXMapComponentBase : UIView, RNMBXMapComponent {
103138
_mapCallbacks.append(callback)
104139
}
105140
}
106-
107-
public func waitForStyleLoad() -> Bool {
108-
return false
109-
}
110-
141+
111142
public func addToMap(_ map: RNMBXMapView, style: Style) {
112143
_mapCallbacks.forEach { callback in
113144
callback(map)
114145
}
115146
_mapCallbacks = []
116147
_map = map
117148
}
118-
149+
119150
public func removeFromMap(_ map: RNMBXMapView, reason: RemovalReason) -> Bool {
120151
_mapCallbacks = []
121152
_map = nil
122153
return true
123154
}
124155
}
125156

157+
/// Base class for components that require MapView to be non-nil
158+
open class RNMBXMapAndMapViewComponentBase : UIView, RNMBXMapAndMapViewComponent {
159+
private weak var _map: RNMBXMapView! = nil
160+
private var _mapCallbacks: [(RNMBXMapView) -> Void] = []
161+
162+
weak var map : RNMBXMapView? {
163+
return _map;
164+
}
165+
166+
func withMapView(_ callback: @escaping (_ mapView: MapView) -> Void) {
167+
withRNMBXMapView { mapView in
168+
callback(mapView.mapView)
169+
}
170+
}
171+
172+
func withRNMBXMapView(_ callback: @escaping (_ map: RNMBXMapView) -> Void) {
173+
if let map = _map {
174+
callback(map)
175+
} else {
176+
_mapCallbacks.append(callback)
177+
}
178+
}
179+
180+
// Uses default implementation from RNMBXMapComponentProtocol extension
181+
182+
public func addToMap(_ map: RNMBXMapView, mapView: MapView, style: Style) {
183+
_mapCallbacks.forEach { callback in
184+
callback(map)
185+
}
186+
_mapCallbacks = []
187+
_map = map
188+
}
189+
190+
public func removeFromMap(_ map: RNMBXMapView, mapView: MapView, reason: RemovalReason) -> Bool {
191+
_mapCallbacks = []
192+
_map = nil
193+
return true
194+
}
195+
}
196+
126197
@objc(RNMBXCamera)
127-
open class RNMBXCamera : RNMBXMapComponentBase {
198+
open class RNMBXCamera : RNMBXMapAndMapViewComponentBase {
128199
var cameraAnimator: BasicCameraAnimator?
129200
let cameraUpdateQueue = CameraUpdateQueue()
130201

@@ -519,18 +590,18 @@ open class RNMBXCamera : RNMBXMapComponentBase {
519590
_updateCamera()
520591
}
521592

522-
public override func addToMap(_ map: RNMBXMapView, style: Style) {
523-
super.addToMap(map, style: style)
593+
public override func addToMap(_ map: RNMBXMapView, mapView: MapView, style: Style) {
594+
super.addToMap(map, mapView: mapView, style: style)
524595
map.reactCamera = self
525596
}
526-
527-
public override func removeFromMap(_ map: RNMBXMapView, reason: RemovalReason) -> Bool {
597+
598+
public override func removeFromMap(_ map: RNMBXMapView, mapView: MapView, reason: RemovalReason) -> Bool {
528599
if (reason == .StyleChange) {
529600
return false
530601
}
531602

532-
map.mapView.viewport.removeStatusObserver(self)
533-
return super.removeFromMap(map, reason:reason)
603+
mapView.viewport.removeStatusObserver(self)
604+
return super.removeFromMap(map, mapView: mapView, reason: reason)
534605
}
535606

536607
@objc public func moveBy(x: Double, y: Double, animationMode: NSNumber?, animationDuration: NSNumber?, resolve: @escaping RCTPromiseResolveBlock, reject: @escaping RCTPromiseRejectBlock) {

ios/RNMBX/RNMBXCustomLocationProvider.swift

Lines changed: 18 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -3,77 +3,71 @@ import MapboxMaps
33
let TAG = "RNMBXCustomLocationProvider"
44

55
@objc
6-
public class RNMBXCustomLocationProvider: UIView, RNMBXMapComponent {
6+
public class RNMBXCustomLocationProvider: UIView, RNMBXMapAndMapViewComponent {
77
var map: RNMBXMapView? = nil
8-
8+
99
let changes : PropertyChanges<RNMBXCustomLocationProvider> = PropertyChanges()
10-
10+
1111
enum Property: String {
1212
case coordinate
1313
case heading
14-
14+
1515
func apply(locationProvider: RNMBXCustomLocationProvider) {
1616
switch self {
1717
case .coordinate: locationProvider.applyCoordinate()
1818
case .heading: locationProvider.applyHeading()
1919
}
2020
}
2121
}
22-
22+
2323
@objc
2424
public var coordinate: [Double] = [] {
2525
didSet { changed(.coordinate) }
2626
}
27-
27+
2828
@objc
2929
public var heading: NSNumber = 0.0 {
3030
didSet { changed(.heading) }
3131
}
32-
32+
3333
func changed(_ property: Property) {
3434
changes.add(name: property.rawValue, update: property.apply)
3535
}
36-
36+
3737
@objc
3838
override public func didSetProps(_ props: [String]) {
3939
if customLocationProvider != nil {
4040
changes.apply(self)
4141
}
4242
}
43-
43+
4444
var customLocationProvider: CustomLocationProvider? = nil
4545
#if RNMBX_11
4646
#else
4747
var defaultLocationProvider: LocationProvider?
4848
#endif
4949

50-
public func addToMap(_ map: RNMBXMapView, style: Style) {
50+
public func addToMap(_ map: RNMBXMapView, mapView: MapView, style: Style) {
5151
self.map = map
52-
if let mapView = map.mapView {
53-
installCustomeLocationProviderIfNeeded(mapView: mapView)
54-
changes.apply(self)
55-
}
52+
installCustomeLocationProviderIfNeeded(mapView: mapView)
53+
changes.apply(self)
5654
}
57-
55+
5856
private func applyCoordinate() {
5957
updateCoordinate(latitude: coordinate[1], longitude: coordinate[0])
6058
}
61-
59+
6260
private func applyHeading() {
6361
updateHeading(heading: heading.doubleValue)
6462
}
65-
66-
public func removeFromMap(_ map: RNMBXMapView, reason: RemovalReason) -> Bool {
67-
if let mapView = map.mapView {
68-
removeCustomLocationProvider(mapView: mapView)
69-
}
63+
64+
public func removeFromMap(_ map: RNMBXMapView, mapView: MapView, reason: RemovalReason) -> Bool {
65+
removeCustomLocationProvider(mapView: mapView)
7066
self.map = nil
7167
return true
7268
}
7369

74-
public func waitForStyleLoad() -> Bool {
75-
false
76-
}
70+
// Uses default implementation from RNMBXMapComponentProtocol extension (returns false)
7771
}
7872

7973
#if RNMBX_11

ios/RNMBX/RNMBXImages.swift

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,11 +74,8 @@ open class RNMBXImages : UIView, RNMBXMapComponent {
7474
}
7575

7676
// MARK: - RNMBXMapComponent
77+
// Uses default implementation from RNMBXMapComponentProtocol extension (returns false)
7778

78-
public func waitForStyleLoad() -> Bool {
79-
return false
80-
}
81-
8279
public func addToMap(_ map: RNMBXMapView, style: Style) {
8380
self.style = style
8481
imageManager = map.imageManager

ios/RNMBX/RNMBXInteractiveElement.swift

Lines changed: 22 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,63 +1,70 @@
11
import MapboxMaps
22

33
@objc
4-
public class RNMBXInteractiveElement : UIView, RNMBXMapComponent {
4+
public class RNMBXInteractiveElement : UIView, RNMBXMapAndMapViewComponent {
55
weak var map : RNMBXMapView? = nil
6+
weak var mapView : MapView? = nil
67

78
static let hitboxDefault = 44.0
89

910
@objc public var draggable: Bool = false
10-
11+
1112
@objc public var hasPressListener: Bool = false
12-
13+
1314
@objc public var hitbox : [String:NSNumber] = [
1415
"width": NSNumber(value: hitboxDefault),
1516
"height": NSNumber(value: hitboxDefault)
1617
]
17-
18+
1819
@objc public var id: String! = nil {
1920
willSet {
2021
if id != nil && newValue != id {
2122
Logger.log(level:.warn, message: "Changing id from: \(optional: id) to \(optional: newValue), changing of id is not supported")
22-
if let map = map { removeFromMap(map, reason: .ComponentChange) }
23+
if let map = map, let mapView = mapView {
24+
removeFromMap(map, mapView: mapView, reason: .ComponentChange)
25+
}
2326
}
2427
}
2528
didSet {
2629
if oldValue != nil && oldValue != id {
27-
if let map = map { addToMap(map, style: map.mapboxMap.style) }
30+
if let map = map, let mapView = mapView {
31+
addToMap(map, mapView: mapView, style: mapView.mapboxMap.style)
32+
}
2833
}
2934
}
3035
}
31-
36+
3237
@objc public var onDragStart: RCTBubblingEventBlock? = nil
33-
38+
3439
@objc public var onPress: RCTBubblingEventBlock? = nil
35-
40+
3641
func getLayerIDs() -> [String] {
3742
return []
3843
}
3944

4045
func isDraggable() -> Bool {
4146
return draggable
4247
}
43-
48+
4449
func isTouchable() -> Bool {
4550
return hasPressListener
4651
}
47-
48-
// MARK: - RNMBXMapComponent
49-
public func addToMap(_ map: RNMBXMapView, style: Style) {
52+
53+
// MARK: - RNMBXMapAndMapViewComponent
54+
public func addToMap(_ map: RNMBXMapView, mapView: MapView, style: Style) {
5055
if (self.id == nil) {
5156
Logger.log(level: .error, message: "id is required on \(self) but not specified")
5257
}
5358
self.map = map
59+
self.mapView = mapView
5460
}
5561

56-
public func removeFromMap(_ map: RNMBXMapView, reason: RemovalReason) -> Bool {
62+
public func removeFromMap(_ map: RNMBXMapView, mapView: MapView, reason: RemovalReason) -> Bool {
5763
self.map = nil
64+
self.mapView = nil
5865
return true
5966
}
60-
67+
6168
public func waitForStyleLoad() -> Bool {
6269
return true
6370
}

0 commit comments

Comments
 (0)