Remove/re-add layers when source-related properties change (#571)

* Remove/re-add layers when source-related properties change

Two cases:

- Fix #570 - remove/re-add layer when `source`, `source-layer`, or
  `type` properties are changed.
- Remove all dependent layers before removing a source; if the source is
  being removed and re-added, then the corresponding layers are also
  added again afterward. (See mapbox/mapbox-gl-js#3621 (comment))

* Add explainer comment

* Fix lint

* Slightly better readability

* One more comment

lib/diff.js

@@ -97,7 +97,7 @@ var operations = {
 };
 
 
-function diffSources(before, after, commands) {
+function diffSources(before, after, commands, sourcesRemoved) {
     before = before || {};
     after = after || {};
 
@@ -108,6 +108,7 @@ function diffSources(before, after, commands) {
         if (!before.hasOwnProperty(sourceId)) continue;
         if (!after.hasOwnProperty(sourceId)) {
             commands.push({ command: operations.removeSource, args: [sourceId] });
+            sourcesRemoved[sourceId] = true;
         }
     }
 
@@ -120,6 +121,7 @@ function diffSources(before, after, commands) {
             // no update command, must remove then add
             commands.push({ command: operations.removeSource, args: [sourceId] });
             commands.push({ command: operations.addSource, args: [sourceId, after[sourceId]] });
+            sourcesRemoved[sourceId] = true;
         }
     }
 }
@@ -216,6 +218,17 @@ function diffLayers(before, after, commands) {
         // no need to update if previously added (new or moved)
         if (clean[layerId] || isEqual(beforeLayer, afterLayer)) continue;
 
+        // If source, source-layer, or type have changes, then remove the layer
+        // and add it back 'from scratch'.
+        if (!isEqual(beforeLayer.source, afterLayer.source) || !isEqual(beforeLayer['source-layer'], afterLayer['source-layer']) || !isEqual(beforeLayer.type, afterLayer.type)) {
+            commands.push({ command: operations.removeLayer, args: [layerId] });
+            // we add the layer back at the same position it was already in, so
+            // there's no need to update the `tracker`
+            insertBeforeLayerId = tracker[tracker.lastIndexOf(layerId) + 1];
+            commands.push({ command: operations.addLayer, args: [afterLayer, insertBeforeLayerId] });
+            continue;
+        }
+
         // layout, paint, filter, minzoom, maxzoom
         diffLayerPropertyChanges(beforeLayer.layout, afterLayer.layout, commands, layerId, null, operations.setLayoutProperty);
         diffLayerPropertyChanges(beforeLayer.paint, afterLayer.paint, commands, layerId, null, operations.setPaintProperty);
@@ -273,6 +286,7 @@ function diffStyles(before, after) {
     var commands = [];
 
     try {
+        // Handle changes to top-level properties
         if (!isEqual(before.version, after.version)) {
             return [{ command: operations.setStyle, args: [after] }];
         }
@@ -300,8 +314,36 @@ function diffStyles(before, after) {
         if (!isEqual(before.light, after.light)) {
             commands.push({ command: operations.setLight, args: [after.light] });
         }
-        diffSources(before.sources, after.sources, commands);
-        diffLayers(before.layers, after.layers, commands);
+
+        // Handle changes to `sources`
+        // If a source is to be removed, we also--before the removeSource
+        // command--need to remove all the style layers that depend on it.
+        var sourcesRemoved = {};
+
+        // First collect the {add,remove}Source commands
+        var removeOrAddSourceCommands = [];
+        diffSources(before.sources, after.sources, removeOrAddSourceCommands, sourcesRemoved);
+
+        // Push a removeLayer command for each style layer that depends on a
+        // source that's being removed.
+        // Also, exclude any such layers them from the input to `diffLayers`
+        // below, so that diffLayers produces the appropriate `addLayers`
+        // command
+        var beforeLayers = [];
+        if (before.layers) {
+            before.layers.forEach(function (layer) {
+                if (sourcesRemoved[layer.source]) {
+                    commands.push({ command: operations.removeLayer, args: [layer.id] });
+                } else {
+                    beforeLayers.push(layer);
+                }
+            });
+        }
+        commands = commands.concat(removeOrAddSourceCommands);
+
+        // Handle changes to `layers`
+        diffLayers(beforeLayers, after.layers, commands);
+
     } catch (e) {
         // fall back to setStyle
         console.warn('Unable to compute style diff:', e);

test/diff.js

@@ -225,5 +225,72 @@ t('diff', function (t) {
       }] }
     ], 'multiple light properties change');
 
+    t.deepEqual(diffStyles({
+        layers: [ { id: 'a', source: 'source-one' } ]
+    }, {
+        layers: [ { id: 'a', source: 'source-two' } ]
+    }), [
+        { command: 'removeLayer', args: ['a'] },
+        { command: 'addLayer', args: [{ id: 'a', source: 'source-two' }, undefined] }
+    ], 'updating a layer\'s source removes/re-adds the layer');
+
+    t.deepEqual(diffStyles({
+        layers: [{ id: 'a', type: 'fill' }]
+    }, {
+        layers: [{ id: 'a', type: 'line' }]
+    }), [
+        { command: 'removeLayer', args: ['a'] },
+        { command: 'addLayer', args: [{ id: 'a', type: 'line' }, undefined] }
+    ], 'updating a layer\'s type removes/re-adds the layer');
+
+    t.deepEqual(diffStyles({
+        layers: [{ id: 'a', source: 'a', 'source-layer': 'layer-one' }]
+    }, {
+        layers: [{ id: 'a', source: 'a', 'source-layer': 'layer-two' }]
+    }), [
+        { command: 'removeLayer', args: ['a'] },
+        { command: 'addLayer', args: [{ id: 'a', source: 'a', 'source-layer': 'layer-two' }, undefined] }
+    ], 'updating a layer\'s source-layer removes/re-adds the layer');
+
+    t.deepEqual(diffStyles({
+        layers: [
+            { id: 'b' },
+            { id: 'c' },
+            { id: 'a', type: 'fill' }
+        ]
+    }, {
+        layers: [
+            { id: 'c' },
+            { id: 'a', type: 'line' },
+            { id: 'b' }
+        ]
+    }), [
+        { command: 'removeLayer', args: ['b'] },
+        { command: 'addLayer', args: [{id: 'b'}, undefined] },
+        { command: 'removeLayer', args: ['a'] },
+        { command: 'addLayer', args: [{ id: 'a', type: 'line' }, 'b'] }
+    ], 'pair respects layer reordering');
+
+    t.deepEqual(diffStyles({
+        sources: { foo: { data: 1 }, bar: {} },
+        layers: [
+            { id: 'a', source: 'bar' },
+            { id: 'b', source: 'foo' },
+            { id: 'c', source: 'bar' }
+        ]
+    }, {
+        sources: { foo: { data: 2 }, bar: {} },
+        layers: [
+            { id: 'a', source: 'bar' },
+            { id: 'b', source: 'foo' },
+            { id: 'c', source: 'bar' }
+        ]
+    }), [
+        { command: 'removeLayer', args: ['b'] },
+        { command: 'removeSource', args: ['foo'] },
+        { command: 'addSource', args: ['foo', { data: 2 }] },
+        { command: 'addLayer', args: [{id: 'b', source: 'foo'}, 'c'] }
+    ], 'changing a source removes and re-adds dependent layers');
+
     t.end();
 });