mapbox / mapbox-gl-geocoder

Geocoder control for mapbox-gl-js using Mapbox Geocoding API
https://mapbox.com/mapbox-gl-js/example/mapbox-gl-geocoder/
ISC License
362 stars 181 forks source link

Clarify/rework "localGeocoderOnly" option wrt external geocoder #393

Open stevage opened 3 years ago

stevage commented 3 years ago

So, there are now three potential sources of geocoder results: Mapbox, local, custom external.

There is one flag to switch off Mapbox and only use local. (Implicitly, if localGeocoder or externalGeocoder is undefined, they won't be used. ) But there doesn't seem to be a way to have:

Could I suggest maybe renaming localGeocoderOnly to useMapbox, defaulting to true? Or some other option where not using Mapbox is not coupled to also using local results, and not external results.

andrewharvey commented 3 years ago

I agree, that in light of now the external geocoder feature, localGeocoderOnly is not a good design, at the time it made sense, but since things have evolved more organically here we haven't had the benefit of considering all features together in the design.

I see "renaming" as deprecating localGeocoderOnly and introducing useMapboxGeocoder. That should cover any combination of scenarios.

It can also come down to what this library is intended as, a general purpose search control for mapbox-gl-js or a UI for the Mapbox Geocoding API. I don't think the goal is to be a general purpose control for any backend geocoding API, but instead mostly designed for the Mapbox Geocoding API with a few options to help supplement that geocoder for specific use cases. A control that could be used with other backend out of the box is probably best done as a fork or entirely separate project in my opinion.

stevage commented 3 years ago

It can also come down to what this library is intended as, a general purpose search control for mapbox-gl-js or a UI for the Mapbox Geocoding API.

My suggestion would be:

andrewharvey commented 3 years ago

Depends where Mapbox want to take it, I see goal 1 as being core, but with 2 the danger is you could end up with down the track with a bigger maintainable maintenance burden for things that are not core to the project. Though this is aside this issue, the specific ask here I think is completely reasonable and useful.

We can cross that bridge when it comes up, so far no one has proposed making a generic control.

stevage commented 3 years ago

you could end up with down the track with a bigger maintainable burden for things that are not core to the project.

Which would compromise goal 1 :)

stevage commented 3 years ago

To add a requirement here, it would also be useful if the localGeocoder function could somehow indicate whether or not an API request is still needed. There are cases where a query perfectly matches a locally coded result. In this case, it is just noise (and expense) to follow through with the API call.