React-leaflet: Moving popup off screen causes `Maximum update depth exceeded` error when storing viewport in state

[chokn]

Bug report

Before opening an issue, make sure to read the contributing guide and understand this is a bug tracker, not a support platform.

Please make sure to check the following boxes before submitting an issue.\
Issues opened without using this template will be closed unless they have a good reason not to follow this template.

• [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

When moving popup off screen, App should not crash.

Actual behavior

When moving popup off the screen, App crashes with

Uncaught Error: Maximum update depth exceeded.
...
The above error occurred in the <Popup> component:

and
Uncaught TypeError: Cannot read property 'classList' of undefined

Steps to reproduce

This happens with a map where the viewport is stored in state and updated with the onViewportChanged prop.

1. Make a popup show by clicking on a marker.
2. Pan the map so that the popup moves off the screen .

The app crashes with the errors described above.

CodePen reproducing issue

[PaulLeCam]

This is not a bug in React-Leaflet but rather the combination of bad factors in the code logic.

First thing is to understand what Leaflet does: by default it will auto-pan the map so any open popup keeps being visible, so by dragging a popup out of the viewport, you're triggering this behavior.
Then there is your marker rendering logic of let post = [marker.lat, marker.lng]; that creates a new position for every render, meaning the markers are getting updated and re-rendered every time the map is rendered.
Finally, you're using onViewportChange which is continuously triggered as the map is panning. You should only use this handler for side-effects, never to set state affecting the map. Here you should use onViewportChanged (notice the trailing d) that is only fired once, after the map has finished panning.

So depending on the behavior you want to apply, you have two options:

• Disable auto-panning the map when an open popup is out of the viewport, setting autoPan={false} on your Popup elements.
• Make sure your markers don't re-render while the map is animating to keep the popup inside the viewport, by making their position constant, ex:

const markerList = [
  {
    position: [17.441013, 78.391796],
    name: "ABC Hospitals",
    info: 10
  },
  ...
]

...

const markers = markerList.map((marker, index) => {
  return (
    <Marker position={marker.position} ...>
    ...

Either way, don't use onViewportChange to trigger effects on the map or you'll always have the risk of having race conditions between Leaflet's animations and React-Leaflet rendering logic.

[chokn]

Thanks for taking the time to explain all of this. I was able to fix my issue and make the rendering more efficient. I will close this issue.

As an aside, react-map-gl seems to have taken the approach of making their map component a stateless component, which seems like it might avoid problems associated with having the state of the map in both the Leaflet object and React state.

[PaulLeCam]

React-Leaflet is also stateless, but Leaflet maintains its own state so this is what can cause issues between the app logic and Leaflet.

Using the Map's viewport is useful if you want to drive the viewport from your app, but in most cases using center and zoom props is likely a simpler alternative.

[rogerroelofs]

Actually this can still happen for the reverse case. Using the provided codepen, drag the map so the pins are near the top of the map and then click one. The map attempts to scroll down to bring the popup into view. This causes the onViewportChanged function to fire multiple times. Setting the state here triggers a render which triggers more onViewportChanged calls and the loop spirals until react kills it. Here's another codePen that shows the same problem.