react-leaflet-draw icon indicating copy to clipboard operation
react-leaflet-draw copied to clipboard

The handleChange function is fired multiple times during edit or delete operations.

Open abid211051 opened this issue 9 months ago • 4 comments

I am working on a Next.js project with react-leaflet-draw. I have a server option to save and load polygon shape data, but I’m facing an issue with the onEdited and onDeleted events. The onCreated event is called only once, no matter how many polygon shapes I draw. However, every time I edit or delete a polygon, the handleChange function is called one additional time, increasing with each edit or delete operation. This behavior is very strange.

onCreated

Image

onEdited

Image

Image

onDeleted

Image

abid211051 avatar Apr 03 '25 14:04 abid211051

Having the same issue. The useEffect function that handles the event handling calls (EditControl.js#L36-L67) appears to be updating multiple times, creating multiple event handlers. This should be countered by the cleanup function, but this doesn't seem to be performed...

robinmolen avatar Apr 11 '25 13:04 robinmolen

I think i know what the problem is. At first the event handlers are connected to the map instance:

https://github.com/alex3165/react-leaflet-draw/blob/2fec00409d5f98e07f177c22c2e8de94fbb43ab8/src/EditControl.js#L40C5-L50C6
for (const key in eventHandlers) {
      map.on(eventHandlers[key], (evt) => {
        let handlers = Object.keys(eventHandlers).filter(
          (handler) => eventHandlers[handler] === evt.type
        );
        if (handlers.length === 1) {
          let handler = handlers[0];
          props[handler] && props[handler](evt);
        }
      });
    }

This creates a wrapper function, that does some weird magic (haven't looked yet at the git blame) and eventually executes the handler passed from the props. (So a function that calls the handler)

Later, during the cleanup, the handler itself is unmounted:

https://github.com/alex3165/react-leaflet-draw/blob/2fec00409d5f98e07f177c22c2e8de94fbb43ab8/src/EditControl.js#L59C7-L63C8
for (const key in eventHandlers) {
        if (props[key]) {
          map.off(eventHandlers[key], props[key]);
        }
      }

So the problem is: the map.on() and map.off() need to receive the exact same function, to be able to unmount it (kinda similar to how window.addeventlistener and window.removeeventlistener work). Because the function passed to map.on() is a wrapper around the props[handler], the map.off() cannot unmount the handler (causing more and more handlers for the same events).

So, there are two solutions: either the weird magic wrapper that is used for the map.on() also needs to be used for the map.off(), OR the map.on() should just use the props[handler] as handler (without the wrapper)

robinmolen avatar Apr 11 '25 14:04 robinmolen

I've made a PR for this https://github.com/alex3165/react-leaflet-draw/pull/193, @alex3165 if you can take a look :)

robinmolen avatar Apr 14 '25 08:04 robinmolen

onDeleted fires multiple times and never destroys old events. Bug is still there. Here is mine fixed 3 months ago. Please check:

 ```
function EditControl(props) {
  const context = useLeafletContext();
  const drawRef = useRef(null);
  const handlersRef = useRef({});

  const onDrawCreate = useCallback((e) => {
    const { onCreated } = props;
    const container = context.layerContainer || context.map;
    container.addLayer(e.layer);
    onCreated?.(e);
  }, [context, props.onCreated]);

  // Main effect for control setup
  useEffect(() => {
    const { map } = context;
    if (!map || !L.Control.Draw) return; // Safety check

    // Clear previous handlers
    Object.entries(handlersRef.current).forEach(([key, handler]) => {
      map.off(eventHandlers[key], handler);
    });

    // Setup new handlers
    const newHandlers = {};
    Object.keys(eventHandlers).forEach((key) => {
      if (props[key]) {
        const handler = (e) => props[key](e);
        newHandlers[key] = handler;
        map.on(eventHandlers[key], handler);
      }
    });
    handlersRef.current = newHandlers;

    map.on(L.Draw.Event.CREATED, onDrawCreate);
    
    drawRef.current = new L.Control.Draw(createDrawOptions(props, context));
    map.addControl(drawRef.current);
    props.onMounted?.(drawRef.current);

    return () => {
      map.off(L.Draw.Event.CREATED, onDrawCreate);
      Object.entries(handlersRef.current).forEach(([key, handler]) => {
        map.off(eventHandlers[key], handler);
      });
      drawRef.current?.remove();
    };
  }, [context, onDrawCreate]);

  // Effect for draw options changes
  useEffect(() => {
    const { map } = context;
    if (!map || !drawRef.current) return;

    const prevOptions = drawRef.current.options;
    const newOptions = createDrawOptions(props, context);

    if (!isEqual(prevOptions, newOptions)) {
      drawRef.current.remove();
      drawRef.current = new L.Control.Draw(newOptions);
      map.addControl(drawRef.current);
    }
  }, [props.draw, props.edit, props.position]);

  return null;
}
``` 

darkodavidovic avatar Oct 09 '25 22:10 darkodavidovic