Merge pull request #4197 from plotly/misc-mapbox-fixes

A few mapbox fixes

Author: etpinard

src/plot_api/plot_api.js

@@ -2657,6 +2657,7 @@ function react(gd, data, layout, config) {
     function addFrames() { return exports.addFrames(gd, frames); }
 
     gd = Lib.getGraphDiv(gd);
+    helpers.clearPromiseQueue(gd);
 
     var oldFullData = gd._fullData;
     var oldFullLayout = gd._fullLayout;

src/plots/mapbox/layers.js

@@ -145,17 +145,26 @@ proto.removeLayer = function() {
 
 proto.dispose = function() {
     var map = this.subplot.map;
-    map.removeLayer(this.idLayer);
-    map.removeSource(this.idSource);
+    if(map.getLayer(this.idLayer)) map.removeLayer(this.idLayer);
+    if(map.getSource(this.idSource)) map.removeSource(this.idSource);
 };
 
 function isVisible(opts) {
+    if(!opts.visible) return false;
+
     var source = opts.source;
 
-    return opts.visible && (
-        Lib.isPlainObject(source) ||
-        ((typeof source === 'string' || Array.isArray(source)) && source.length > 0)
-    );
+    if(Array.isArray(source) && source.length > 0) {
+        for(var i = 0; i < source.length; i++) {
+            if(typeof source[i] !== 'string' || source[i].length === 0) {
+                return false;
+            }
+        }
+        return true;
+    }
+
+    return Lib.isPlainObject(source) ||
+        (typeof source === 'string' && source.length > 0);
 }
 
 function convertOpts(opts) {

src/traces/choropleth/hover.js

@@ -58,6 +58,7 @@ function makeHoverInfo(pointData, trace, pt) {
     if(trace.hovertemplate) return;
 
     var hoverinfo = pt.hi || trace.hoverinfo;
+    var loc = String(pt.loc);
 
     var parts = (hoverinfo === 'all') ?
         attributes.hoverinfo.flags :
@@ -72,10 +73,10 @@ function makeHoverInfo(pointData, trace, pt) {
     var text = [];
 
     if(hasIdAsNameLabel) {
-        pointData.nameOverride = pt.loc;
+        pointData.nameOverride = loc;
     } else {
         if(hasName) pointData.nameOverride = trace.name;
-        if(hasLocation) text.push(pt.loc);
+        if(hasLocation) text.push(loc);
     }
 
     if(hasZ) {

test/jasmine/tests/choroplethmapbox_test.js

@@ -589,6 +589,44 @@ describe('@noCI Test choroplethmapbox hover:', function() {
         nums: '10.000',
         name: 'PROP::New York',
         evtPts: [{location: 'NY', z: 10, pointNumber: 0, curveNumber: 0, properties: {name: 'New York'}}]
+    }, {
+        desc: 'with "typeof number" locations[i] and feature id (in *name* label case)',
+        patch: function() {
+            var fig = Lib.extendDeep({}, require('@mocks/mapbox_choropleth-raw-geojson.json'));
+            fig.data.shift();
+            fig.data[0].locations = [100];
+            fig.data[0].geojson.id = 100;
+            return fig;
+        },
+        nums: '10',
+        name: '100',
+        evtPts: [{location: 100, z: 10, pointNumber: 0, curveNumber: 0}]
+    }, {
+        desc: 'with "typeof number" locations[i] and feature id (in *nums* label case)',
+        patch: function() {
+            var fig = Lib.extendDeep({}, require('@mocks/mapbox_choropleth-raw-geojson.json'));
+            fig.data.shift();
+            fig.data[0].locations = [100];
+            fig.data[0].geojson.id = 100;
+            fig.data[0].hoverinfo = 'location+name';
+            return fig;
+        },
+        nums: '100',
+        name: 'trace 0',
+        evtPts: [{location: 100, z: 10, pointNumber: 0, curveNumber: 0}]
+    }, {
+        desc: 'with "typeof number" locations[i] and feature id (hovertemplate case)',
+        patch: function() {
+            var fig = Lib.extendDeep({}, require('@mocks/mapbox_choropleth-raw-geojson.json'));
+            fig.data.shift();
+            fig.data[0].locations = [100];
+            fig.data[0].geojson.id = 100;
+            fig.data[0].hovertemplate = '### %{location}<extra>%{location} ###</extra>';
+            return fig;
+        },
+        nums: '### 100',
+        name: '100 ###',
+        evtPts: [{location: 100, z: 10, pointNumber: 0, curveNumber: 0}]
     }];
 
     specs.forEach(function(s) {

test/jasmine/tests/mapbox_test.js

@@ -942,6 +942,80 @@ describe('@noCI, mapbox plots', function() {
         .then(done);
     }, LONG_TIMEOUT_INTERVAL);
 
+    it('@gl should not wedge graph after reacting to invalid layer', function(done) {
+        Plotly.react(gd, [{type: 'scattermapbox'}], {
+            mapbox: {
+                layers: [{ source: 'invalid' }]
+            }
+        })
+        .then(function() {
+            fail('The above Plotly.react promise should be rejected');
+        })
+        .catch(function() {
+            expect(gd._promises.length).toBe(1, 'has 1 rejected promise in queue');
+        })
+        .then(function() {
+            return Plotly.react(gd, [{type: 'scattermapbox'}], {
+                mapbox: {
+                    layers: [{
+                        sourcetype: 'vector',
+                        sourcelayer: 'contour',
+                        source: 'mapbox://mapbox.mapbox-terrain-v2'
+                    }]
+                }
+            });
+        })
+        .then(function() {
+            expect(gd._promises.length).toBe(0, 'rejected promise has been cleared');
+
+            var mapInfo = getMapInfo(gd);
+            expect(mapInfo.layoutLayers.length).toBe(1, 'one layer');
+            expect(mapInfo.layoutSources.length).toBe(1, 'one layer source');
+        })
+        .catch(failTest)
+        .then(done);
+    }, LONG_TIMEOUT_INTERVAL);
+
+    it('@gl should not attempt to remove non-existing layer sources', function(done) {
+        function _assert(msg, exp) {
+            return function() {
+                var layerList = gd._fullLayout.mapbox._subplot.layerList;
+                expect(layerList.length).toBe(exp, msg);
+            };
+        }
+
+        Plotly.react(gd, [{type: 'scattermapbox'}], {
+            mapbox: { layers: [{}] }
+        })
+        .then(_assert('1 visible:false layer', 1))
+        .then(function() {
+            return Plotly.react(gd, [{type: 'scattermapbox'}], {
+                mapbox: { layers: [] }
+            });
+        })
+        .then(_assert('no layers', 0))
+        .catch(failTest)
+        .then(done);
+    }, LONG_TIMEOUT_INTERVAL);
+
+    it('@gl should validate layout layer input', function(done) {
+        Plotly.newPlot(gd, [{type: 'scattermapbox'}], {
+            mapbox: {
+                layers: [{
+                    sourcetype: 'raster',
+                    source: ['']
+                }]
+            }
+        })
+        .then(function() {
+            var mapInfo = getMapInfo(gd);
+            expect(mapInfo.layoutLayers.length).toBe(0, 'no on-map layer');
+            expect(mapInfo.layoutSources.length).toBe(0, 'no map source');
+        })
+        .catch(failTest)
+        .then(done);
+    }, LONG_TIMEOUT_INTERVAL);
+
     it('@gl should be able to update the access token', function(done) {
         Plotly.relayout(gd, 'mapbox.accesstoken', 'wont-work').catch(function(err) {
             expect(gd._fullLayout.mapbox.accesstoken).toEqual('wont-work');