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

support new Static API and rename current as Classic. #139

Closed
andrewharvey opened this issue Apr 6, 2017 · 2 comments
Closed

support new Static API and rename current as Classic. #139

andrewharvey opened this issue Apr 6, 2017 · 2 comments

Comments

@andrewharvey
Copy link
Collaborator

andrewharvey commented Apr 6, 2017

The current MapboxStatic service is for Static (Classic). I'd like to implement the newer Static service, but not sure what approach to take.

  1. Rename the current MapboxStatic service to MapboxStaticClassic and drop in the new Static API into MapboxStatic service.
  2. Leave the MapboxStatic service but rename the getStaticURL method to getStaticClassicURL and drop the new Static API into getStaticURL.
  3. Drop support for the Static (Classic) API and replace the current implementation to support the new Static API.

All break backwards compatibility though and I'm not sure of a a better solution which doesn't. So what's the preferred approach here?

@1ec5
Copy link

1ec5 commented Apr 10, 2017

In mapbox/MapboxStatic.swift#54, I took advantage of the similarity between both APIs’ overlay options to essentially implement (2) with different map ID/style URL options but identical overlay options.

As far as backwards compatibility, this library is technically pre-1.0 (v1.0.0-beta6 is the latest), so I guess backwards compatibility could be broken with minimal fuss, right?

@andrewharvey
Copy link
Collaborator Author

andrewharvey commented Apr 10, 2017

@1ec5 Agreed the identical overlay options makes it easy, I'll tidy up what I've done and PR approach (2).

Working on it in https://github.com/andrewharvey/mapbox-sdk-js/tree/new-static-api, just need to test, add unit tests before I PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants