-
-
Notifications
You must be signed in to change notification settings - Fork 655
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
Remove mapbox-specific code #21
Conversation
@petrsloup one key question here is if we want to merge this before or after our "most compatible possible with mapbox-gl-js" initial release, on the one hand, it will necessarily break mapbox URLs etc. Another question is: Are we sure we want to completely disable mapbox? It might make sense for many of our users to continue being mapbox customers, but using a free sofware way to access mapbox features? Probably not, but worth thinking briefly about. |
I would say we want to ship this after the initial version (v1.13). |
Our user case would be that we would use our own tile layers, but there are cases for using the mapbox layers especially for quick proof of concepts and tests? Do you envisage the getting started docs to use MapTiler or Mapzen or another vector tile service instead of Mapbox? |
I'm not in favor of this change. Given what I've read above
I think backwards compatibility should be a high priority for a while, or we risk fracturing the community even further. This change seems to be inconsistent with the stated desire
It seems to me that removing the ability to use Mapbox data services breaks compatibility, and if people can avoid triggering this code by self-hosting data, it should be moot. Edit:
In my understanding, this change implies that it cannot be used at all with any Mapbox services without breaking their TOS. |
I think we should introduce absolute minimum changes in our first official release, doing only what is the minimum required from the legal perspective. I suspect this just means rebranding, without any changes to the API (my understanding is that API compatibility is an exception to trademark law, but IANAL). The only possible exception is to disable telemetry to Mapbox if only non-Mapbox data sources are used. Last I checked the telemetry is not being sent for non-MB sources already, but we should make sure. Going forward, I would prefer our users to have the option to use Mapbox data sources if they so choose, but we clearly cannot guarantee that if users use MapLibre code with Mapbox data, they won't violate Mapbox's terms of service. Lastly, I think it would make sense to rename |
Closes #6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The style changes don't really make sense, as the underlying component that generates the DOM elements with class .mapboxgl-ctrl-attrib
haven't been touched.
Also probably we'd want to make it possible to interface with Mapbox APIs via maplibre-gl-js
in the long run?
This PR seems a bit reckless IMO.
* mapboxgl.accessToken = myAccessToken; | ||
* @see [Display a map](https://www.mapbox.com/mapbox-gl-js/examples/) | ||
*/ | ||
get accessToken(): ?string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might we want to keep these, so that it would still be possible to interface with Mapbox APIs via maplibre-gl-js?
@@ -536,12 +536,6 @@ a.mapboxgl-ctrl-logo.mapboxgl-compact { | |||
text-decoration: underline; | |||
} | |||
|
|||
/* stylelint-disable-next-line selector-class-pattern */ | |||
.mapboxgl-ctrl-attrib .mapbox-improve-map { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The component that makes this stuff is still in the repo https://github.com/maplibre/maplibre-gl-js/blob/f87090b70b8a6fbde1a779c44933ce32bffc5d2c/src/ui/control/attribution_control.js . Did you mean to delete that stuff too? This styling seems to still make sense to keep, maybe we could just rename it to not refer to Mapbox.
A case against removing Mapbox code: |
It's worse than that. There's nothing whatever to say that using it as it stands is not a violation of the terms of service. |
@HarelM this is outdated, right? |
Yes, but it has code changes that I think should make it in, like the removal of the access token. |
@petrsloup what's preventing this from being merged? |
Closing in favor of #293. |
This PR remove the mapbox-specific portions of the code:
The most notable change is the "removal" of
/src/utils/mapbox.js
, which contains the following warning:But after this change, the maplibre-gl-js cannot be easily used with mapbox data (the
mapbox://
urls will not work any more), so we should be okay. Maybe @nyurik has more insight into this ?I've tested this a little (it works with styles from MapTiler Cloud API -- vector tiles/raster tiles/DEM/sprites/fonts, so everything should be loading okay). But I wasn't able to run the full test suite at the moment.
Please add commits to this branch if you find any more mapbox-specific pieces of code.