mapbox / mapbox/mapbox-directions-swift
RouteOptions and Waypoint should be structs
- Dominant language
- Swift
- Stars
- 206
- Forks
- 99
- Avg merge
- 7h 54m
- Merged PRs (30d)
- 3
Description
RouteOptions, MatchOptions, Waypoint, and Tracepoint should all be structs, not classes. We pass these types around as if they’re grab bags of values; ideally, they would have no identity apart from their values.
## Background
This library has always represented the set of options for an API request as an object, even though a struct would be a more correct representation of pass-by-value semantics. We kept RouteOptions as a class because the library had to be compatible with Objective-C code; Swift structs don’t bridge to Objective-C. From there, we started using inheritance to add flexibility to the request functionality. To support the Map Matching API, #236 introduced DirectionsOptions as an abstract base class for RouteOptions and MatchOptions. Meanwhile, mapbox/mapbox-navigation-ios#531 added a subclass of RouteOptions, NavigationRouteOptions, to provide good default values optimized for turn-by-turn navigation.
## Implementation notes
Dropping Objective-C compatibility in #382 gives us an opportunity to convert RouteOptions and MatchOptions into structs. It’s more complicated than that, because waypoints also had to be converted to structs at the same time. It turns out that the Directions API has two different kinds of waypoints, with disjoint sets of information: one that goes in requests and another that goes in responses.
NavigationRouteOptions does nothing more than set some default values in its convenience initializer. We can safely replace it with an extension method on RouteOptions.
We allow and sometimes encourage developers to subclass NavigationRouteOptions to fine-tune the Directions API requests that come from the application. For example, mapbox/mapbox-navigation-ios-examples#91 illustrates how to use beta query parameters that the MapboxDirections library doesn’t formally support yet. Structs can’t be subclassed in Swift, so we need a different solution for customizations that MapboxDirections doesn’t or can’t support yet.
## Next steps
#388 replaces the Waypoint class with separate structs for request and response waypoints, with a goal of eventually converting RouteOptions to a struct as well. This PR has the potential to complicate client code, particularly in the navigation SDK, because client code now has to distinguish between request and response waypoints and potentially convert between the two at various times. Unfortunately, we were unable to prioritize landing this PR for a long time due to unrelated constraints, but the use of classes for input types remains an impediment to adopting more server API features and fully supporting client customization.
/cc @mapbox/navigation-ios
Contributor guide
Research direction
Start by reviewing RouteOptions, MatchOptions, Waypoint, Tracepoint, and NavigationRouteOptions, then read the context from PR #388. Map the request and response waypoint requirements and the existing customization use cases before determining the replacement API. Done means these option and waypoint types use value semantics without losing the described navigation customization support.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- swift
- Domain
- api
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 25/100