feat: remove deprecated RNMBXMapView's mapView & mapboxMap APIs #3963
Conversation
In our App, the deprecated APIs (`RNMBXMapView.mapView` & `RNMBXMapView.mapboxMap`) cause the app to crash because rnmapbox is trying to unwrap them while they are still `nil`. This PR removes those two deprecated APIs and changes all their usage to the safe `withMapView` & `withMapboxMap` API calls.
PRESIDENT810
temporarily deployed
to
CI with Mapbox Tokens
GitHub Actions
Inactive
PRESIDENT810
had a problem deploying
to
CI with Mapbox Tokens
GitHub Actions
Failure
PRESIDENT810
temporarily deployed
to
CI with Mapbox Tokens
GitHub Actions
Inactive
PRESIDENT810
temporarily deployed
to
CI with Mapbox Tokens
GitHub Actions
Inactive
PRESIDENT810
had a problem deploying
to
CI with Mapbox Tokens
GitHub Actions
Failure
PRESIDENT810
temporarily deployed
to
CI with Mapbox Tokens
GitHub Actions
Inactive
PRESIDENT810
had a problem deploying
to
CI with Mapbox Tokens
GitHub Actions
Failure
PRESIDENT810
changed the title
|
@PRESIDENT810 thanks much for the PR Unfortunately it's a bit complicated. Like in case of remove? withMapView could keep stuff alive and create a leak. So one thing we have to be carefully withMapView is leaks the other is concurrency, as the code will be called from other thread. And can cause deadlock and other issues. Do you perhaps has stack traces? So we can address those cases, where you hit by the nil mapboxMap? |
|
@mfazekas Here's the stacktrace I can see the crash is caused by 5th closure in RNMBXMapView.setupEvents with "Swift runtime failure: Unexpectedly found nil while implicitly unwrapping an optional value" but I can't see what triggers it. For leaks, I think I used |
PRESIDENT810
had a problem deploying
to
CI with Mapbox Tokens
GitHub Actions
Failure
PRESIDENT810
had a problem deploying
to
CI with Mapbox Tokens
GitHub Actions
Failure
PRESIDENT810
had a problem deploying
to
CI with Mapbox Tokens
GitHub Actions
Failure
PRESIDENT810
had a problem deploying
to
CI with Mapbox Tokens
GitHub Actions
Failure
PRESIDENT810
had a problem deploying
to
CI with Mapbox Tokens
GitHub Actions
Failure
PRESIDENT810
had a problem deploying
to
CI with Mapbox Tokens
GitHub Actions
Failure
PRESIDENT810
had a problem deploying
to
CI with Mapbox Tokens
GitHub Actions
Failure
|
ping @mfazekas |
|
|
||
| guard let mapboxMap = map.mapboxMap else { | ||
| return false | ||
| map.withMapboxMap { [weak self] _mapboxMap in |
There was a problem hiding this comment.
Not sure about this. Maybe we should store the style we've added ourself to? Doesn't make much sense to wait for a style here. It could be other style, then we've added to, so...
Also not sure if a race like this is possible:
- item added to map with style A
- style removed from map
- item tries to removeFromMap so it's waits for style
- style B is added to map
- item is added to style B
- withMapboxMap is executed item removed from style B
this can't happen as 6.) will be called before 4.) but it's something I need to think about on every change like this.
| if let center = camera.center { | ||
| try center.validate() | ||
| } | ||
| map.withMapView { _mapView in |
| } | ||
|
|
||
| map.mapView.viewport.removeStatusObserver(self) | ||
| map.withMapView { _mapView in |
There was a problem hiding this comment.
Same as the other remove, we don't want to try to remove from other mapViewpot we've added ourselves to
| self.map = map | ||
| if let mapView = map.mapView { | ||
| installCustomeLocationProviderIfNeeded(mapView: mapView) | ||
| map.withMapView{ _mapView in |
There was a problem hiding this comment.
So we have a valid style, we should have a map too. Maybe add a map parameter as well?!
| source.url = url | ||
| self.doUpdate { (style) in | ||
| try! style.setSourceProperty(for: id, property: "url", value: url) | ||
| self.map?.withMapboxMap { [weak self] _mapboxMap in |
There was a problem hiding this comment.
same concern here as well. We've added ourself to style. So we don't want to wait for it again. Maybe we should save the style here as well
| resolver(["data": NSNumber(value: result)]) | ||
| } else { | ||
| resolver(nil) | ||
| view.withMapboxMap { [weak view] _mapboxMap in |
| ]) | ||
| case .failure(let error): | ||
| rejecter("queryRenderedFeaturesInRect", "failed to query features", error) | ||
| map.withMapboxMap { [weak map] _mapboxMap in |
| rejecter( | ||
| "querySourceFeatures", | ||
| "failed to query source features: \(error.localizedDescription)", error) | ||
| map.withMapboxMap { _mapboxMap in |
|
@PRESIDENT810 thanks again, and sorry for taking this long. So generally there is 2 cases in the changes. |
PRESIDENT810
had a problem deploying
to
CI with Mapbox Tokens
GitHub Actions
Failure
PRESIDENT810
had a problem deploying
to
CI with Mapbox Tokens
GitHub Actions
Failure
PRESIDENT810
had a problem deploying
to
CI with Mapbox Tokens
GitHub Actions
Failure
PRESIDENT810
had a problem deploying
to
CI with Mapbox Tokens
GitHub Actions
Failure
PRESIDENT810
had a problem deploying
to
CI with Mapbox Tokens
GitHub Actions
Failure
PRESIDENT810
had a problem deploying
to
CI with Mapbox Tokens
GitHub Actions
Failure
PRESIDENT810
had a problem deploying
to
CI with Mapbox Tokens
GitHub Actions
Failure
@mfazekas Crashlytics - Stack tracePlatform: appleIssue: fe3bc65935115ec9828d90410e1d4584Session: 95f85b7fd0464ae6bb83bd846f315998_DNE_0_v2Date: Thu Nov 06 2025 15:15:20 GMT+0800 (中国标准时间)Crashed: com.apple.main-thread com.apple.uikit.eventfetch-thread com.google.firebase.crashlytics.MachExceptionServer com.apple.NSURLConnectionLoader com.apple.CFSocket.private Thread com.facebook.react.JavaScript hades com.mapbox.common.Worker 1 com.mapbox.common.Worker 2 com.mapbox.common.Worker 3 com.mapbox.common.Worker 4 com.mapbox.common.location.sharedRunloop com.mapbox.common.AssetFileSource com.mapbox.common.TileStoreFileSource com.mapbox.common.LocalFileSource com.mapbox.common.OnlineFileSource com.mapbox.common.ResourceLoaderThread Thread Thread Thread Thread Thread Thread |
…e-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): 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>
3 participants
Description
In our App, the deprecated APIs (
RNMBXMapView.mapView&RNMBXMapView.mapboxMap) cause the app to crash because rnmapbox is trying to unwrap them while they are stillnil.This PR removes those two deprecated APIs and changes all their usage to the safe
withMapView&withMapboxMapAPI calls. After this fix our app no longer crashes.Checklist
CONTRIBUTING.mdyarn generatein the root folder/exampleapp./example)Screenshot OR Video
Component to reproduce the issue you're fixing
Kinda hard since the crash is triggered by an edge case in our app and we don't know how to create a minimal reproducing example yet but after I patched this package our app no longer crashes.