PaulLeCam / react-leaflet

React components for Leaflet maps
https://react-leaflet.js.org
Other
5.18k stars 888 forks source link

MapContainer does not cleanup when unmounted #1001

Closed tbowmo closed 2 years ago

tbowmo commented 2 years ago

Bug report in v4

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.

Expected behavior

React-leaflet cleans up the leaflet-map container when MapContainer is unmounted.

Actual behavior

While debugging an error in my site with "Map container is already initialized", I went through the code of React-Leaflet, and in particular MapContainer.

MapContainer tries to remove the leaflet map here, I tried inserting a console.log, writing out the content of context, this is always undefined in my case, so it never calls map.remove() to remove the map when the component is unmounted.

While debugging, I also placed a simple console.log('here') inside the mapRef useCallback method, just before if, and it is never executed (console.log is never written)

voxelias commented 2 years ago

If I understand right, as a result of this, I have fullscreencontrol duplicated on every re-draw

PaulLeCam commented 2 years ago

Indeed the context wasn't captured, it should be fixed in v4.0.1, please try it out!

voxelias commented 2 years ago

@PaulLeCam It seems that external controls are not being removed from the map on component redraw still.

voxelias commented 2 years ago

Snímek obrazovky z 2022-06-25 16-45-06

<MapContainer...>
      <FullScreenControl />
</MapContainer>

The code of FullScreenControl component:

import L from 'leaflet'
import { createControlComponent } from '@react-leaflet/core'
import 'leaflet.fullscreen'
import 'react-leaflet-fullscreen/dist/styles.css'

function createFullScreenControl(props) {
  return L.control.fullscreen(props)
}

const FullScreenControl = createControlComponent(createFullScreenControl)

export default FullScreenControl
PaulLeCam commented 2 years ago

@Lowentwickler please open a dedicated issue with a running example.

tbowmo commented 2 years ago

@PaulLeCam

I tried 4.0.1, but the problem persists, context is still null, when entering the useEffect cleanup upon unmount, (singlestepping through the code in debug mode)

PaulLeCam commented 2 years ago

It's not according to my tests, adding logs to Leaflet's map.remove() shows the call is successfully made, so I won't investigate this further. If you still have issues, this might be specific to your code rather than the library.

barbalex commented 1 year ago

I have an app that has been using react-leaflet forever.

Recently I have code-split the app to reduce the size of the js code needed for first render. The app is basically huge. To do this I have loaded the most important components using React.lazy and included them in Suspense components (https://reactjs.org/docs/code-splitting.html#reactlazy).

Nothing else was changed.

The result was great: the lighthouse score for performance went from 36% to 99%.

Since then we regulary see the "Map container is already initialized" error. As far as I can remember it happended often when the map component itself was lazy loaded and included in a Suspense component. Since I removed that one it happens irregularly. No Idea what provokes it.

Seems to me that something must be happening that prevents react-leaflet from running map.remove().

I guess I will have to remove all lazy calls again.

I tried to reproduce on StackBlitz. But: The lazyness happens mainly inside the routing (https://github.com/barbalex/apf2/blob/77558473f0c891dc1c8cd988fb1d23152b8ba86d/src/components/Router/index.jsx). I can't simulate that in StackBlitz. Or can I?

And I have a feeling, the hierarchical routing using createBrowserRouter (https://reactrouter.com/en/main/routers/create-browser-router#createbrowserrouter) is part of the reason this happens.