Directions API. Url provider

RingerJK commented 4 years ago

Description

In terms of Navigation for Android exists 2 types of Navigation(Router), it's Offline and Online. Both of them needs Route-URL provider.(Offline and Online are different modules , have own dependency graph and might be used independent).

Online Router's Route URL is provided by MapboxDirections that is a part of services-core library(under the hood is Retrofit(+OkHttp) library)

Problem

Offline Router cannot consume url from services-core because it brings "network" dependencies like Retrofit and OkHttp(they shouldn't be in Offline navigation). Offline Router has own route-url provider RouteUrl The main issue here: two classes where the same piece of logic exist.

Solution

keep service-core library interfaces as it is
create additional library (like router-url-provider) that depends on service-directions-model and (if java doesn't have clever approach for creating url) additional java library

How dependency tree looks now

Suggestion

Pros

router-url-provider might be used independently on service-core 👍

cc @mapbox/navigation-android @mapbox/maps-android @mapbox/navigation-api

zugaldia commented 4 years ago

Tagging @langsmith for initial review - you probably are the most familiar with the current set up.

langsmith commented 4 years ago

Offline Router cannot consume url from services-core because it brings "network" dependencies like Retrofit and OkHttp(they shouldn't be in Offline navigation). Offline Router has own route-url provider RouteUrl The main issue here: two classes where the same piece of logic exist.

@RingerJK, is this still an issue? I ask because maybe things have changed in https://github.com/mapbox/mapbox-navigation-android since you opened this ticket. If so, is it an absolute blocker for navigation work?

RingerJK commented 4 years ago

@langsmith yes, it's still actual issue. It's not a blocker, because we have opportunity to get url for Offline routing via internal implementation(RouteUrl), but would be great to resolve current issue to avoid problems with url in future

RingerJK commented 4 years ago

@langsmith @zugaldia I can take it if you're never mind

zugaldia commented 4 years ago

@RingerJK should we address this issue for 1.0, or can I wait until after?

RingerJK commented 4 years ago

@zugaldia It's not a blocker for 1.0, event for 1.1. This Issue regarding resolve duplicating code in Navigation SDK and Directions API. Of course we might wait till 1.0 👍

mapbox / mapbox-java