maplibre / maplibre-navigation-ios

MapLibre Navigation SDK for iOS
Other
29 stars 27 forks source link

Deprecate some hardcoded mapbox styles, and replace those that remain with maplibre styles. (+docs!) #45

Closed michaelkirk closed 1 month ago

michaelkirk commented 1 month ago

Fixes #42

Description

Following up on https://github.com/maplibre/maplibre-navigation-ios/issues/42#issuecomment-2114511460

In the mapbox days, it arguably made sense to have hardcoded defaults that referenced mapbox servers. I think we can agree that in the maplibre world it no longer makes sense to reference these mapbox services.

Perhaps more controversially: Post maplibre, I think it makes less sense to have any default service provider at all. Instead, we should surface API's that require the implementor to pass in an explicit backend tile provider. I've introduced some new API's to that end, and deprecated some of the existing API's which utilize a default tile source.

Screenshot of previous API along with the newly proposed deprecation messages

Screenshot 2024-05-22 at 17 16 56

For example:

let styleURL = AppConfig().tileserverStyleUrl

// previous broken way
// `styles` were optional. The default value specified mapbox, which was broken.
let vc = MapboxNavigation.NavigationViewController(
    for: route,
)

// previous way that works, but is now proposed to being deprecated, because `styles` isn't actually optional in practice.
let style = DayStyle()
style.mapStyleURL = styleURL
let vc = MapboxNavigation.NavigationViewController(
    for: route,
    styles: [style]
)

// new way 1: pass in non-optional style
let style = DayStyle(styleURL: styleURL)
let vc = MapboxNavigation.NavigationViewController(
    for: route,
    dayStyle: dayStyle
)

// new way 2: pass in only URL, using default colors for night and day UI components
let vc = MapboxNavigation.NavigationViewController(
    for: route,
    dayStyleURL: styleURL
)

We still do make available some "demo" styles that use https://demotiles.maplibre.org, but I've intentionally made it explicit that it's a demo - DayStyle(demoStyle: ()). See below for the alternatives rationale.

Alternatives considered

Swap out mapbox tile server for malibre tile server

I could have maintained the current APIs where specifying styles is optional, and simply swapped the mapbox tiles out for maplibre demo tiles. However, my understanding of the maplibre project is, unlike mapbox, maplibre doesn't (currently anyway) intend to host production map services, and so maplibre libraries should direct people towards finding their own service providers, even at some cost to the "everything just works" experience. I think providing an easy way to use the demo services for POC, while trying to nudge users to proper external tile hosts is the right balance. But I'm open to learning more about this if people feel differently.

Another alternative that would be a smaller code change would be to keep the existing init, but make styles in NavigationViewController.init(styles:) non-optional. This would still be a semver breaking change, but an arguably simpler change for users to adapt to. My understanding is that, in practice, the user would pass in 0, 1, or 2 styles. Accepting an array of 3 or more styles is not useful as far as I know. Plus I think the old API where "the second style should be the night style" is a bit obscure. Since we were breaking the API anyway, I decided to codify the expectations.

Info for reviewers

I'm very new to the maplibre native ecosystem, so consider this PR a proposal open for comment and changes. I look forward to your feedback.

michaelkirk commented 1 month ago

Whoops, converting to a draft while I address a missed TODO.

michaelkirk commented 1 month ago

Re-marking this as ready for review.

I brushed up against some things that could use more attention (CarPlay and weird NavigationViewController tests). I didn't fix everything, but I think everything changed is either neutral or an improvement.

michaelkirk commented 1 month ago

Ok, Changelog entry added. Curious if people care about mitigating the breaking change, but otherwise I think it's good to go.