search

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
369 stars 179 forks source link

Debounce option #185

Open matthewlein opened 5 years ago

matthewlein commented 5 years ago

Hello,

I am using a custom endpoint, and I could use an option to set the debounce ms as an option, to try to reduce the load on that endpoint.

I'm happy to make the PR, it seems like it will be just 2 lines and documentation.

https://github.com/mapbox/mapbox-gl-geocoder/blob/b69197a6852d7ca6337e9efef9e687efab70b786/lib/index.js#L58-L67 https://github.com/mapbox/mapbox-gl-geocoder/blob/b69197a6852d7ca6337e9efef9e687efab70b786/lib/index.js#L164

andrewharvey commented 5 years ago

Personally I don't mind that being configurable, however recently I've been thinking if this would be better to be throttle rather than debounce.

My reasoning is, if I keep on typing within the 200ms debounce, the request will never be sent until I finish typing. As a user I keep on typing since I haven't seen any suggestions yet.

Whereas using throttle, at least I'll start seeing some results show up, even if they are outdated by a few chars, I'd rather see some suggestions start to popup up instead of none.

The reason I bring this up is if we add a public debounceWait option, but then swap to throttle internally it creates a breaking API change, so best if we can decide on this first. I'll break this out into a separate ticket.

matthewlein commented 5 years ago

Yep, I agree with that UX, although it would end up hitting my server even more, which is what I'm trying to control. In that regard, choosing between debounce or throttle, and also setting the timeout is the most useful option.

lordinkavu commented 3 years ago

Hello, Is this enhancement under progress ? Having the option to set debounce interval would be very useful for a project I'm currently working on. If the throttle option is not being implemented then is it possible to include the feature ? I am ready to make a PR if needed. Thanks!

andrewharvey commented 3 years ago

@lordinkavu, @lordinkavu started a PR at #436, but per the comments there this feature needs discussion.