Slight mods to PR 6800 (#6809)

* mods

* code review changes

* revert parameter

* fix comment

* comment, revert formatting

* link and spacing

* more

* flatten updateable

* rename variable

---------

Co-authored-by: wayofthefuture <[email protected]>

Author: wayofthefuture

src/source/geojson_source.test.ts

@@ -977,7 +977,7 @@ describe('GeoJSONSource.shoudReloadTile', () => {
 
         expect(result).toBeTruthy();
     });
-    
+
     test('returns false when tile contains a feature that is being removed but was never added', async () => {
         const diff: GeoJSONSourceDiff = {remove: [0]};
         let shouldReloadTileOptions: GeoJSONSourceShouldReloadTileOptions = undefined;
@@ -1030,7 +1030,7 @@ describe('GeoJSONSource.shoudReloadTile', () => {
 
         expect(result).toBe(false);
     });
-    
+
     test('handles string feature ids and returns no bounds since feature does not exist', async () => {
         const diff: GeoJSONSourceDiff = {remove: ['abc']};
 
@@ -1060,4 +1060,4 @@ describe('GeoJSONSource.shoudReloadTile', () => {
         expect(shouldReloadTileOptions).toBeUndefined();
     });
 
-});
+});

src/source/geojson_source.ts

@@ -417,7 +417,8 @@ export class GeoJSONSource extends Evented implements Source {
                 this.fire(new Event('dataabort', {dataType: 'source'}));
                 return;
             }
-            const affectedBounds = this._applyDiffIfNeeded(diff);
+            const affectedGeometries = this._applyDiffToSource(diff);
+            const shouldReloadTileOptions = this._getShouldReloadTileOptions(affectedGeometries);
 
             let resourceTiming: PerformanceResourceTiming[] = null;
             if (result.resourceTiming && result.resourceTiming[this.id]) {
@@ -432,7 +433,7 @@ export class GeoJSONSource extends Evented implements Source {
             // although GeoJSON sources contain no metadata, we fire this event to let the TileManager
             // know its ok to start requesting tiles.
             this.fire(new Event('data', {...eventData, sourceDataType: 'metadata'}));
-            this.fire(new Event('data', {...eventData, sourceDataType: 'content', shouldReloadTileOptions: affectedBounds !== undefined ? {affectedBounds} : undefined}));
+            this.fire(new Event('data', {...eventData, sourceDataType: 'content', shouldReloadTileOptions}));
         } catch (err) {
             this._isUpdatingWorker = false;
             if (this._removed) {
@@ -449,14 +450,15 @@ export class GeoJSONSource extends Evented implements Source {
     }
 
     /**
-     * Apply a diff to the source data and return the bounds of the affected geometries.
-     * @param diff - The diff to apply.
-     * @returns The bounds of the affected geometries, undefined if the diff is not applicable or all geometries are affected.
+     * Apply a diff to this source's data and return the affected feature geometries.
+     * @param diff - The {@link GeoJSONSourceDiff} to apply.
+     * @returns The affected geometries, or undefined if the diff is not applicable or all geometries are affected.
      */
-    private _applyDiffIfNeeded(diff: GeoJSONSourceDiff): LngLatBounds[] | undefined {
+    private _applyDiffToSource(diff: GeoJSONSourceDiff): GeoJSON.Geometry[] | undefined {
         if (!diff) {
             return undefined;
         }
+
         const promoteId = typeof this.promoteId === 'string' ? this.promoteId : undefined;
 
         // Lazily convert `this._data` to updateable if it's not already
@@ -472,13 +474,26 @@ export class GeoJSONSource extends Evented implements Source {
         }
         const affectedGeometries = applySourceDiff(this._data.updateable, diff, promoteId);
 
-        if (this._options.cluster || diff.removeAll) {
+        if (diff.removeAll || this._options.cluster) {
             return undefined;
         }
-            
-        return affectedGeometries
+
+        return affectedGeometries;
+    }
+
+    /**
+     * Get options for use in determining whether to reload a tile based on the modified features.
+     * @param affectedGeometries - The feature geometries affected by the update.
+     * @returns A {@link GeoJSONSourceShouldReloadTileOptions} object which contains an array of affected bounds caused by the update.
+     */
+    private _getShouldReloadTileOptions(affectedGeometries: GeoJSON.Geometry[]): GeoJSONSourceShouldReloadTileOptions | undefined {
+        if (!affectedGeometries) return undefined;
+
+        const affectedBounds = affectedGeometries
             .filter(Boolean)
             .map(g => getGeoJSONBounds(g));
+
+        return {affectedBounds};
     }
 
     /**
@@ -492,7 +507,8 @@ export class GeoJSONSource extends Evented implements Source {
         if (tile.state === 'unloaded') {
             return false;
         }
-        // Update the tile if it WILL NOW contain an updated feature.
+
+        // Update the tile if contained or will contain an updated feature.
         const {buffer, extent} = this.workerOptions.geojsonVtOptions;
         const tileBounds = tileIdToLngLatBounds(
             tile.tileID.canonical,
@@ -503,6 +519,7 @@ export class GeoJSONSource extends Evented implements Source {
                 return true;
             }
         }
+
         return false;
     }
 

src/source/geojson_source_diff.ts

@@ -106,55 +106,66 @@ export function isUpdateableGeoJSON(data: GeoJSON.GeoJSON | undefined, promoteId
 }
 
 export function toUpdateable(data: UpdateableGeoJSON, promoteId?: string) {
-    const result = new Map<GeoJSONFeatureId, GeoJSON.Feature>();
+    const updateable = new Map<GeoJSONFeatureId, GeoJSON.Feature>();
+
+    // empty updateable
     if (data == null || data.type == null) {
-        // empty result
-    } else if (data.type === 'Feature') {
-        result.set(getFeatureId(data, promoteId)!, data);
-    } else {
-        for (const feature of data.features) {
-            result.set(getFeatureId(feature, promoteId)!, feature);
-        }
+        return updateable;
+    }
+
+    if (data.type === 'Feature') {
+        updateable.set(getFeatureId(data, promoteId)!, data);
+        return updateable;
+    }
+
+    for (const feature of data.features) {
+        updateable.set(getFeatureId(feature, promoteId)!, feature);
     }
 
-    return result;
+    return updateable;
 }
 
 /**
- * Mutates updateable and applies a GeoJSONSourceDiff. Operations are processed in a specific order to ensure predictable behavior:
+ * Mutates updateable and applies a {@link GeoJSONSourceDiff}. Operations are processed in a specific order to ensure predictable behavior:
  * 1. Remove operations (removeAll, remove)
  * 2. Add operations (add)
  * 3. Update operations (update)
- * @returns an array of geometries that were affected by the diff
+ * @returns an array of geometries that were affected by the diff - with the exception of removeAll which does not track any affected geometries.
  */
 export function applySourceDiff(updateable: Map<GeoJSONFeatureId, GeoJSON.Feature>, diff: GeoJSONSourceDiff, promoteId?: string): GeoJSON.Geometry[] {
     const affectedGeometries: GeoJSON.Geometry[] = [];
+
     if (diff.removeAll) {
         updateable.clear();
     }
     else if (diff.remove) {
         for (const id of diff.remove) {
-            if (updateable.has(id)) {
-                affectedGeometries.push(updateable.get(id).geometry);
-                updateable.delete(id);
-            }
+            const existing = updateable.get(id);
+            if (!existing) continue;
+
+            affectedGeometries.push(existing.geometry);
+            updateable.delete(id);
         }
     }
 
     if (diff.add) {
         for (const feature of diff.add) {
             const id = getFeatureId(feature, promoteId);
-            if (id != null) {
-                affectedGeometries.push(feature.geometry);
-                updateable.set(id, feature);
-            }
+            if (id == null) continue;
+
+            // we are allowed to replace duplicate features with the same id, so need to check for existing
+            const existing = updateable.get(id);
+            if (existing) affectedGeometries.push(existing.geometry);
+
+            affectedGeometries.push(feature.geometry);
+            updateable.set(id, feature);
         }
     }
 
     if (diff.update) {
         for (const update of diff.update) {
-            let feature = updateable.get(update.id);
-            if (!feature) continue;
+            const existing = updateable.get(update.id);
+            if (!existing) continue;
 
             const changeGeometry = !!update.newGeometry;
 
@@ -167,7 +178,7 @@ export function applySourceDiff(updateable: Map<GeoJSONFeatureId, GeoJSON.Featur
             if (!changeGeometry && !changeProps) continue;
 
             // clone once since we'll mutate
-            feature = {...feature};
+            const feature = {...existing};
             updateable.set(update.id, feature);
             affectedGeometries.push(feature.geometry);