search

nazka / map-gl-js-spiderfy

Spiderfy plugin for mapbox-gl and maplibre-gl.
BSD 3-Clause "New" or "Revised" License
36 stars 5 forks source link

Issue with Spiderfy unspiderfyAll: Breaking Errors When Toggling Layers #26

Open dejandiklic opened 2 months ago

dejandiklic commented 2 months ago

When attempting to clean up spiderfy with unspiderfyAll, after toggling one layer, I expected it to work as intended. Instead, if I toggle on my layer, apply spiderfy to it, then toggle it off and on again, I encounter breaking errors from the library.

Steps to reproduce;

  1. Initialize spiderfy on a Mapbox GL map.
  2. Toggle on desired layer
  3. Apply spiderfy to a clustered layer.
  4. Toggle off desider layer.
  5. Call unspiderfyAll
  6. Toggle on desider layer.
  7. Click on cluster.
  8. Toggle off desider layer.

Source and Layer Information in use:

Expected Behavior: Everything works as expected. I can toggle the layer on and off as much as I want without issues.

Actual Behavior: The app crashes image

I logged the layers and sources after each toggle, and it seems that the map gets the correct state. I think the problem lies somewhere in the library, as the layers seem to duplicate. Specifically, on step 6, I get the following error: image

Logs: After step 1: image

After step 2: image

After step 5: image

After step 6: image

After step 7: image

Versions "@nazka/map-gl-js-spiderfy": "^1.2.3" "mapbox-gl": "^3.4.0"

Relevant code:

`

const spiderfy = useMemo(() => {
    return new Spiderfy(map, {
        spiderLeavesPaint: {
            'text-color': 'black',
        },
        spiderLeavesLayout: fieldPerformerUnClusterLayoutConfig,
    });
}, [map]);

useEffect(() => {
    if (!map) return;
    if (fieldPerformer) {
        dispatch(fetchMapFieldPerformerAsync()).unwrap().then((geojson) => {
            map.addSource('fieldPerformer-source', fieldPerformersSource(geojson as GeoJSON));
            map.addLayer(FieldPerformerUnClusteredLayer);
            map.addLayer(FieldPerformerClusteredLayer);
            spiderfy.applyTo('fieldPerformer-layer-clustered');
        });
    } else {
        spiderfy.unspiderfyAll();
        removeClusteredLayerAndSource(map, 'fieldPerformer');
    }
}, [fieldPerformer, style]);

`

simondriesen commented 2 months ago

I'm not able to reproduce your issue. I will need a working example for me to look into it more.

There should not even be a need to use spiderfy.unspiderfyAll(); when removing a spiderified layer. All should be cleaned automatically.

Here you have my test file for reference (just a map with a toggle button):

<html>
  <head>
    <title>Test</title>
    <link rel="stylesheet" href="https://api.mapbox.com/mapbox-gl-js/v3.6.0/mapbox-gl.css">
    <style>
      body { margin: 0; padding: 0; }
      #map { position: absolute; top: 0; bottom: 0; width: 100%; }
    </style>
  </head>
  <body>
    <div id="map"></div>
    <script type="module">
      import mapboxgl from "https://cdn.skypack.dev/mapbox-gl@3.5.2";
      import Spiderfy from "https://cdn.skypack.dev/@nazka/map-gl-js-spiderfy";

      mapboxgl.accessToken = ""; // Your Mapbox API token

      const map = new mapboxgl.Map({
        container: 'map',
        center: [4.7126, 50.8781],
        zoom: 11.5,
      });
      window.map = map;

      map.on('load', () => {
        map.addSource('markers', {
          'type': 'geojson',
          'data': 'https://raw.githubusercontent.com/nazka/map-gl-js-spiderfy/dev/demo/json/sample-data.json',
          'cluster': true, // must be true
        }); // public toilets in Leuven sample data

        map.loadImage(
        'https://raw.githubusercontent.com/nazka/map-gl-js-spiderfy/dev/demo/img/circle-yellow.png',
        (error, image) => {
          if (error) throw error;
          map.addImage('cluster', image);

          map.addLayer({
            'id': 'markers',
            'type': 'symbol',
            'source': 'markers',
            'layout': {
              'icon-image': 'cluster',
              'icon-allow-overlap': true
            }
          });

          map.addLayer({
          "id": "counters",
          "type": "symbol",
          "source": "markers",
          "layout": {
            "text-field": ["get", "point_count"],
            "text-size": 12,
            "text-allow-overlap": true
          }
        })

          const spiderfy = new Spiderfy(map, {
            onLeafClick: f => console.log(map.getStyle().layers, map.getStyle().sources),
            minZoomLevel: 12,
            zoomIncrement: 2,
          });
          window.spiderfy = spiderfy;
          spiderfy.applyTo('markers');
        });
      });
    </script>

    <!-- Toggle logic -->
    <button onclick="toggleLayer()" style="position: absolute;">toggle layer</button>
    <script>
      function toggleLayer() {
        const map = window.map;
        const spiderfy = window.spiderfy;
        if (map.getLayer('markers')) {
          map.removeLayer('markers');
          map.removeLayer('counters');
          map.removeSource('markers');
          spiderfy.unspiderfyAll();
          console.log(map.getStyle().layers, map.getStyle().sources)
        } else {
          map.addSource('markers', {
            type: 'geojson',
            data: 'https://raw.githubusercontent.com/nazka/map-gl-js-spiderfy/dev/demo/json/sample-data.json',
            cluster: true,
          });
          map.addLayer({
            id: 'markers',
            type: 'symbol',
            source: 'markers',
            layout: {
              'icon-image': 'cluster',
              'icon-allow-overlap': true,
            },
          });
          map.addLayer({
            id: 'counters',
            type: 'symbol',
            source: 'markers',
            layout: {
              'text-field': ['get', 'point_count'],
              'text-size': 12,
              'text-allow-overlap': true,
            },
          });
          console.log(map.getStyle().layers, map.getStyle().sources)
        }
      }
    </script>
  </body>
</html>
dejandiklic commented 2 months ago

Sorry for the late reply. Following the same logic you used in the example, I was able to fix my issue, which I would now call a misunderstanding of usage rather than a bug. I initially thought I needed to "remove" spiderify and apply it again when removing layers. My intended flow was:

  1. Add layer
  2. Apply spiderify to it
  3. Remove layer
  4. Remove spiderify

This would represent a layer toggle. However, upon closer inspection, I realized you only need to add spiderify once and use unspiderfyAll to clean up the map. The correct flow should be:

  1. Add layer
  2. Apply spiderify to it
  3. Remove layer
  4. unspiderfyAll
  5. Add layer
  6. Remove layer
  7. unspiderfyAll

I've created a sandbox that demonstrates the usage I initially thought would work: https://codesandbox.io/p/sandbox/keen-merkle-24flrv?file=%2Fsrc%2FApp.js%3A9%2C1

It might be worth considering adding a destroy or a removeLayer method that takes the same layer passed to the applyTo function. This way, we wouldn't need to keep track of whether spiderify has already been applied.

Thank you for your time and assistance in helping me understand this better. Your insights were very helpful!