Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RouteOptions and Waypoint should be structs #564

Open
1ec5 opened this issue Jul 20, 2021 · 1 comment · May be fixed by #388
Open

RouteOptions and Waypoint should be structs #564

1ec5 opened this issue Jul 20, 2021 · 1 comment · May be fixed by #388
Labels
backwards incompatible changes that break backwards compatibility of public API jira-sync-complete op-ex Refactoring, Tech Debt or any other operational excellence work.
Milestone

Comments

@1ec5
Copy link
Contributor

1ec5 commented Jul 20, 2021

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

@1ec5 1ec5 added backwards incompatible changes that break backwards compatibility of public API op-ex Refactoring, Tech Debt or any other operational excellence work. labels Jul 20, 2021
@1ec5
Copy link
Contributor Author

1ec5 commented Jul 20, 2021

This is one half of #392: that issue also tracks eliminating the DirectionsResult abstract base class, which is also desirable but not as urgent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards incompatible changes that break backwards compatibility of public API jira-sync-complete op-ex Refactoring, Tech Debt or any other operational excellence work.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants