add support for configurable API base URL

[incanus]

I think this fixes #6346. It's fairly identical to how access tokens work in the SDK.

I prefer either (2) or (3) because MGLOfflineStorage is largely asynchronous and therefore easy to use incorrectly.

The DefaultFileSource::setAPIBaseURL() that is called under the hood is synchronous. I think we can expect that if someone changes the API base URL midstream during a download operation, unexpected things will happen. This is the same with the access token. Is that what you meant @1ec5? Am I missing something as to why this can't be this simple?

Builds on @tobrun's work in #6309.

Todo:

• Finish docs
• Mirror to macOS
• Add tests

[mention-bot]

@incanus, thanks for your PR! By analyzing the history of the files in this pull request, we identified @1ec5, @frederoni and @boundsj to be potential reviewers.

[incanus]

Also, I thought it best to add a new singleton class MGLNetworkConfiguration since going at MGLOfflineStorage is non-obvious, particularly when you are not using offline functionality. Much like MGLAccountManager, it's single-purpose and clearly indicates what it's for. It's a bit more API surface area but I think it's the right thing to do.

[incanus]

One item up for debate here is if +[MGLNetworkConfiguration apiBaseURL] should return the default, internal mbgl::util::API_BASE_URL value of (currently) https://api.mapbox.com or just nil in the default case.

To fit the current setup of returning nil, I added proper nullability support in debf157.

[1ec5]

The DefaultFileSource::setAPIBaseURL() that is called under the hood is synchronous. I think we can expect that if someone changes the API base URL midstream during a download operation, unexpected things will happen. This is the same with the access token. Is that what you meant @1ec5?

Yes, that’s what I meant. An Info.plist-based implementation is ideal because the developer has an easy way to set it before MGLOfflineStorage is initialized – in the Info.plist – and because changing the base URL midstream results in undefined behavior.

Also, I thought it best to add a new singleton class MGLNetworkConfiguration since going at MGLOfflineStorage is non-obvious, particularly when you are not using offline functionality. Much like MGLAccountManager, it's single-purpose and clearly indicates what it's for. It's a bit more API surface area but I think it's the right thing to do.

MGLMapView uses MGLOfflineStorage for online functionality too. “Offline” in this context means fetching and caching, that’s all.

I wonder if it’s really necessary to expose a formal API around this setting. I always imagined this setting to be more similar to the telemetry base URL Info.plist entry we had originally (before turning it into a user default): just a simple dictionary read in MGLOfflineStorage right when we initialize the file source, and no API additions whatsoever. The only benefit to a formal API would be that people can set the base URL in their application delegate instead of in the Info.plist, but I still think that’s a risky proposition. As it’s currently designed, the base URL setting is an advanced setting for a very specific use case, not something we would even mention in a guide.

[incanus]

Ok, well this works out of the Info.plist just fine, so I think we can indeed remove the public API—it's easy enough to add if requested in future.

“Offline” in this context means fetching and caching, that’s all.

Right, but that's non-obvious as well 😄

I think I'll just remove the public methods from MGLNetworkConfiguration.

[incanus]

I just got rid of the setter, keeping the getter, in 3ec5510 and included docs in 9f223ba to indicate that this will return any custom API base URL used (nil if none), including from Info.plist, which can aid in any debugging.

[1ec5]

Without a programmatic setter, there’s no point to having a getter, because the developer can just as easily call [[NSBundle mainBundle] objectForInfoDictionaryKey:@"MGLMapboxAPIBaseURL"] themselves.

[incanus]

How about 85206fd then, which just takes the class private now? Note that I've named the (only) header MGLNetworkConfiguration_Private.h and naturally it is not included in Mapbox.h. But it does not declare a class extension; the filename is merely a reminder that this is not public API. Look good?

[1ec5]

Looks fine other than the file name. (We don’t explicitly say “Private” for private-only classes.) This is still more than we actually need for the feature, but I suppose an extra private class couldn’t hurt. MGLNetworkConfiguration could be a nice place to hang reachability stuff off of in the future.

[1ec5]

If there’s no public version of this header, there’s no need for _Private.

[incanus]

5c4aa6b

[incanus]

MGLNetworkConfiguration could be a nice place to hang reachability stuff off of in the future.

My thoughts exactly.

[1ec5]

Qt builds are failing reliably with the following error, which is certainly unrelated to these changes:

-- [Mason] Unpacking package to mason_packages/headers/geojson/0.3.2...
CMake Error at /usr/local/lib/cmake/Qt5Core/Qt5CoreConfig.cmake:15 (message):
  The imported target "Qt5::Core" references the file

     "/usr/local/.//mkspecs/macx-clang"

  but this file does not exist.  Possible reasons include:

  * The file was deleted, renamed, or moved to another location.

  * An install or uninstall procedure did not complete successfully.

  * The installation package was faulty and contained

     "/usr/local/lib/cmake/Qt5Core/Qt5CoreConfigExtras.cmake"

  but not all the files it references.

Call Stack (most recent call first):
  /usr/local/lib/cmake/Qt5Core/Qt5CoreConfigExtras.cmake:50 (_qt5_Core_check_file_exists)
  /usr/local/lib/cmake/Qt5Core/Qt5CoreConfig.cmake:141 (include)
  platform/qt/qt5.cmake:1 (find_package)
  platform/qt/qt.cmake:72 (include)
  platform/qt/config.cmake:1 (include)
  CMakeLists.txt:51 (include)


-- Configuring incomplete, errors occurred!
See also "/Users/vagrant/git/build/qt-macos-x86_64/Debug/CMakeFiles/CMakeOutput.log".
make: *** [build/qt-macos-x86_64/Debug/build.ninja] Error 1

[1ec5]

Filed #6712 about the Qt5 build bot failure.