AutoPan multiple onViewportChanged events

rogerroelofs commented 4 years ago

Bug report

[x] All peer dependencies are installed: React, ReactDOM and Leaflet.
[x] Using a supported version of React and ReactDOM (v16.8.0 minimum).
[x] Using the supported version of Leaflet (v1.6.0 minimum) and its corresponding CSS file is loaded.
[x] Using the latest version of React-Leaflet.
[x] The issue has not already been reported.
[x] Make sure you have followed the quick start guide for Leaflet.
[x] Make sure you have fully read the documentation and that you understand the limitations.

Expected behavior

At the end of any autoPan operation the onViewportChanged event should fire once.

Actual behavior

Multiple onViewportChanged events fire. If the onViewportChanged callback tries to set state a render feedback loop is initiated.

Steps to reproduce

https://codepen.io/rogerroelofs/pen/ZEGqvwx?editors=0011 Open the console. Initiate the autoPan by clicking the marker to show the label. Notice that multiple messages are written to the console. Adding a react setstate call to onViewportChanged will trigger a render which will trigger another onViewportChanged callback which triggers a render... eventually react's error handler steps in and crashes things to the nearest error boundary.

This is actually a bug in leaflet. It is emitting multiple moveend events, but it's a bigger problem for react apps where state changes generate rerenders.

PaulLeCam commented 4 years ago

What you describe as "expected behavior" is not the expected behavior from this library: there should be no expectation that onViewportChanged can't be triggered multiple times, as you described it's based on Leaflet's moveend event. It's up to your application logic to handle things differently as needed.

rogerroelofs commented 4 years ago

@PaulLeCam Thanks for all your work on this. However I don't agree with your assessment of this particular case. It seems reasonable for there to be 1 moveend event per user action. Leaflet autoPan is generating multiple moveend events when the user clicks the pin to open the popup and the popup triggers the autoPan.

I realize it it totally not in react-leaflet code, but because the example code shows setting state in onViewportChanged and doing so can result in a cascade of failures, we at least need a doc change that indicates onViewportChanged can fire multiple times within milliseconds and that client code is expected to handle that.

PaulLeCam commented 4 years ago

It seems reasonable for there to be 1 moveend event per user action

I totally get what you're saying from the app point of view but the reality is that React Leaflet has no knowledge of "user action", it's just an abstraction between your app and Leaflet.

Adding any logic to React Leaflet to "fix" some particular edge case like this comes with the risk of breaking other assumptions that can be made by apps. It's way safer to opt-in to this kind of behavior by adding the logic you need in your app, rather than having it implemented a certain way in React Leaflet and trying to opt-out from it.

Please feel free to update the examples and/or docs if you think it can make the behavior easier to understand, but I think it's best not to make changes to the code.

rogerroelofs commented 4 years ago

Given all the leaflet dynamics the lib already deals with, I can understand your choice.

timkelty commented 4 years ago

@PaulLeCam here's the bit of this I'm struggling with:

Here's vanilla leaflet, with the default autoPan=true on a popup. Click to open the popup, it pans into view, and a single moveend is fired: https://codepen.io/timkelty-the-flexboxer/pen/wvaRGpJ?editors=0011

Now here's the same thing with react-leaflet. This time when you click, it will crash, as it gets in an update/moveEnd feedback loop: https://codepen.io/timkelty-the-flexboxer/pen/mdJaEqB?editors=0011

What seems to be triggering this is updating state in onViewportChanged. If you comment out the this.setState it works fine.

I also just noticed the barely documented debounceMoveend Leaflet option, which I've tried enabling, but haven't noticed any difference: https://leafletjs.com/reference-1.6.0.html#map-invalidatesize

Any advice?

nuschk commented 1 year ago

Now here's the same thing with react-leaflet. This time when you click, it will crash, as it gets in an update/moveEnd feedback loop: https://codepen.io/timkelty-the-flexboxer/pen/mdJaEqB?editors=0011

What seems to be triggering this is updating state in onViewportChanged. If you comment out the this.setState it works fine.

It's been three years since your question, but I'm at a similar point and can try to shed some light on it.

The problem with your react example is the position you're passing into <Marker/> is not stable, hence the marker must internally update() the corresponding leaflet marker with each render. This messes up leaflet's algorithms and leads to an infinite recursion (or something... I didn't delve too much into the detail with your example, mine certainly did that).

Solving it is "easy", just make the position prop stable:

const position = [51.505, -0.09];
...
<Marker position={position}>...</Marker>

Or, when allowed to use lodash, you could wrap it in a memoize() like this:

const makeLatLng = memoize(
  (lat: number, lng: number) => new LatLng(lat, lng)
);
...
<Marker position={makeLatLng(51.505, -0.09)}>...</Marker>

Last, I don't think react-leaflet can solve that for you. I've myself written an abstraction layer over google maps, and these kinds of interactions are always fickle. react-leaflet chose a defensive way, re-rendering whenever something changes, which I think is fine in this case.

rogerroelofs commented 1 year ago

@nuschk Thanks for for documenting your solution. I should have done that when we found our workaround.

PaulLeCam / react-leaflet