Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Create Mapbox GL Start Infrastructure #1225

Closed
bleege opened this issue Apr 7, 2015 · 11 comments
Closed

Create Mapbox GL Start Infrastructure #1225

bleege opened this issue Apr 7, 2015 · 11 comments
Assignees
Labels
iOS Mapbox Maps SDK for iOS
Milestone

Comments

@bleege
Copy link
Contributor

bleege commented Apr 7, 2015

The current Mapbox GL startup system is all tied to MGLMapView. This means that in order for an app developer to use any Mapbox service a MapView has to be built. It also ties Metrics to MapView as well.

Future State Goals

  1. Start Mapbox GL in AppDelegate's didFinishLaunchingWithOptions
    • [MapboxGL startWithAccessToken:@"YOURACCESSTOKENGOESHERE"];
  2. MapboxGL startWithAccessToken's responsibilities
    • Serve as singleton holder of Access Token for all SDK uses
    • Start Metrics services

@mapbox/mobile

@bleege bleege added the iOS Mapbox Maps SDK for iOS label Apr 7, 2015
@bleege bleege added this to the iOS Beta 1 milestone Apr 7, 2015
@1ec5
Copy link
Contributor

1ec5 commented Apr 7, 2015

As you work on this, please keep in mind the XIB/storyboard scenario we enabled in #1070: it should still be possible to set an access token on a view-by-view basis, without overriding anything in the app delegate. Codeless setup is a compelling story (MapKit parity, makes for a good beginner’s guide, shorter demo GIFs), even if we expect most developers to go the programmatic route eventually.

@jfirebaugh
Copy link
Contributor

I'm not sure this is a good idea. A required singleton init method typically makes libraries harder to use.

in order for an app developer to use any Mapbox service a MapView has to be built

What Mapbox services does Mapbox GL provide besides MapViews?

@bleege
Copy link
Contributor Author

bleege commented Apr 23, 2015

A required singleton init method typically makes libraries harder to use.

Generally I agree, but in this particular case it's a very common iOS design pattern is to to initialize 3rd party code at app startup inside the app's AppDelegate. Specifically in the following callback method:

- (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(NSDictionary *)launchOptions

What Mapbox services does Mapbox GL provide besides MapViews?

Currently there are no other services besides MapViews, but if others were to come along in the future (ex: Directions API wrapper) the existing infrastructure would require developers to first create a MapView in order to get access tokens to use. This way would provide a service agnostic one line of code common setup.

@1ec5
Copy link
Contributor

1ec5 commented Apr 23, 2015

A required singleton init method typically makes libraries harder to use.

This is why any singleton setup step needs to be optional: individual top-level view classes like MGLMapView should continue to accept access tokens, so that you can instantiate a working map from within a XIB or stroryboard, or from within a custom React Native component, as @bsudekum is trying to do.

@incanus
Copy link
Contributor

incanus commented Apr 23, 2015

I think that we should hold off on this if the reason is possible future uses, but my understanding was that one might want to trigger metrics without necessary showing a map view at launch — or at all. For that we would need a singleton, and @bleege is right that this is common pattern. HockeyApp and other crash log services do this, as do other map libraries like Google for iOS:

Add the following to your application:didFinishLaunchingWithOptions: method, replacing API_KEY with your API key.

[GMSServices provideAPIKey:@"API_KEY"];

@incanus
Copy link
Contributor

incanus commented Apr 23, 2015

individual top-level view classes like MGLMapView should continue to accept access tokens

Agree here as well, and services like directions and geocoding are probably better served using their own setters too, as MapboxDirections.swift and MapboxGeocoder.swift do.

@bleege
Copy link
Contributor Author

bleege commented Apr 23, 2015

individual top-level view classes like MGLMapView should continue to accept access tokens

Agreed. The goal isn't / wasn't to replace these (nor the ones in Directions and Geocoder), it was to provide a common area for the token to be set so that developers wouldn't have to set it in N number of places in their app as well as to allow Metrics to start without needing to display a MapView at launch.

That said after now seeing the recent updates to MapboxDirections.swift and MapboxGeocoder.swift and how they're being used as separate Legos this isn't likely to be that onerous in the short term. Also Metrics is now started in MGLMapView.commonInit so they'll actually be configured once the MGLMapView is initially created and not require the map to be displayed.

This is a good discussion. Let's pull back on this idea for the time being and come back to it later if the need arrises. 👍

@bleege bleege removed this from the iOS Beta 1 milestone Apr 23, 2015
@incanus
Copy link
Contributor

incanus commented Apr 23, 2015

Also Metrics is now started in MGLMapView.commonInit so they'll actually be configured once the MGLMapView is initially created and not require the map to be displayed.

Isn't this still a problem though? If a map exists on a second tab, it won't be created until that tab is first shown, but metrics might be needed before then?

I think we do need a singleton, but we just weren't talking about the right reasons above. Does that make sense?

@bleege
Copy link
Contributor Author

bleege commented Apr 23, 2015

If a map exists on a second tab, it won't be created until that tab is first shown, but metrics might be needed before then?

Yeah, the more I think about it this does sound like a distinct possibility. Metrics should be on an app wide basis where it's either on or it's off.

I'm going to put this back on the iOS Beta 1 Milestone and go back to building this out.

@bleege bleege added this to the iOS Beta 1 milestone Apr 23, 2015
bleege added a commit that referenced this issue Apr 23, 2015
…pDelegate and exposing MGLMapView.initWithFrame to support it
bleege added a commit that referenced this issue Apr 24, 2015
@bleege
Copy link
Contributor Author

bleege commented Apr 24, 2015

Continued work on this issue this afternoon. I was able to get the MapboxEvents stated inside the MapboxGL shared instance as well as halfway through @1ec5's code review suggestions. Big thanks to @1ec5 for that BTW.... super helpful for learning some of the finer points of Objective-C!

Next up is to finish the other half of the recommendations.

@bleege bleege self-assigned this Apr 29, 2015
@incanus
Copy link
Contributor

incanus commented Apr 29, 2015

Happening in #1329 PR.

@incanus incanus closed this as completed Apr 29, 2015
bleege added a commit that referenced this issue May 4, 2015
…the singleton code into MGLAccountManager.h and .m
bleege added a commit that referenced this issue May 4, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
iOS Mapbox Maps SDK for iOS
Projects
None yet
Development

No branches or pull requests

4 participants