Do not warn about icon-image expressions evaluating to '' (mapbox#9380)

* do not warn on empty icon image, treat it as unset * clean up hasIcon condition in symbol bucket
procore · Mar 18, 2020 · 4b67564 · 4b67564
1 parent 14dafde
commit 4b67564
Show file tree

Hide file tree

Showing 4 changed files with 35 additions and 9 deletions.
diff --git a/src/data/bucket/symbol_bucket.js b/src/data/bucket/symbol_bucket.js
@@ -81,7 +81,7 @@ export type CollisionArrays = {
 export type SymbolFeature = {|
     sortKey: number | void,
     text: Formatted | void,
-    icon: ResolvedImage | void,
+    icon: ?ResolvedImage,
     index: number,
     sourceLayerIndex: number,
     geometry: Array<Array<Point>>,
@@ -415,7 +415,7 @@ class SymbolBucket implements Bucket {
         // this allows us to fire the styleimagemissing event if image evaluation returns null
         // the only way to distinguish between null returned from a coalesce statement with no valid images
         // and null returned because icon-image wasn't defined is to check whether or not iconImage.parameters is an empty object
-        const hasIcon = (iconImage.value.kind !== 'constant' || !!iconImage.value.value) && Object.keys(iconImage.parameters).length > 0;
+        const hasIcon = iconImage.value.kind !== 'constant' || !!iconImage.value.value || Object.keys(iconImage.parameters).length > 0;
         const symbolSortKey = layout.get('symbol-sort-key');
 
         this.features = [];
@@ -453,7 +453,7 @@ class SymbolBucket implements Bucket {
                 }
             }
 
-            let icon: ResolvedImage | void;
+            let icon: ?ResolvedImage;
             if (hasIcon) {
                 // Expression evaluation will automatically coerce to Image
                 // but plain string token evaluation skips that pathway so do the

diff --git a/src/style-spec/expression/definitions/image.js b/src/style-spec/expression/definitions/image.js
@@ -30,13 +30,11 @@ export default class ImageExpression implements Expression {
 
     evaluate(ctx: EvaluationContext) {
         const evaluatedImageName = this.input.evaluate(ctx);
-        let available = false;
 
-        if (ctx.availableImages && ctx.availableImages.indexOf(evaluatedImageName) > -1) {
-            available = true;
-        }
+        const value = ResolvedImage.fromString(evaluatedImageName);
+        if (value && ctx.availableImages) value.available = ctx.availableImages.indexOf(evaluatedImageName) > -1;
 
-        return new ResolvedImage({name: evaluatedImageName, available});
+        return value;
     }
 
     eachChild(fn: (_: Expression) => void) {

diff --git a/src/style-spec/expression/types/resolved_image.js b/src/style-spec/expression/types/resolved_image.js
@@ -18,7 +18,8 @@ export default class ResolvedImage {
         return this.name;
     }
 
-    static fromString(name: string): ResolvedImage {
+    static fromString(name: string): ResolvedImage | null {
+        if (!name) return null; // treat empty values as no image
         return new ResolvedImage({name, available: false});
     }
 

diff --git a/test/unit/ui/map.test.js b/test/unit/ui/map.test.js
@@ -2075,6 +2075,33 @@ test('Map', (t) => {
         });
     });
 
+    t.test('map does not fire `styleimagemissing` for empty icon values', (t) => {
+        const map = createMap(t);
+
+        map.on('load', () => {
+            map.on('idle', () => {
+                t.end();
+            });
+
+            map.addSource('foo', {
+                type: 'geojson',
+                data: {type: 'Point', coordinates: [0, 0]}
+            });
+            map.addLayer({
+                id: 'foo',
+                type: 'symbol',
+                source: 'foo',
+                layout: {
+                    'icon-image': ['case', true, '', '']
+                }
+            });
+
+            map.on('styleimagemissing', ({id}) => {
+                t.fail(`styleimagemissing fired for value ${id}`);
+            });
+        });
+    });
+
     t.end();
 });