feat: remove instanceof Promise checks from geojson_source

The point of this commit is to delay the resolution of the result of
`_getLoadGeoJSONParameters` until the `_isUpdatingWorker` is set; i.e.,
inside `_dispatchWorkerUpdate`. Simply `await`ing the result of
`_getLoadGeoJSONParameters` immediately after the call of the function
breaks the behavior when `setData` or `setClusterOptions` are
consecutively called. It would cause a race condition.

Author: kikuomax

src/source/geojson_source.test.ts

@@ -140,7 +140,7 @@ describe('GeoJSONSource.setData', () => {
         await expect(promise).resolves.toBeDefined();
     });
 
-    test('respects collectResourceTiming parameter on source', () => {
+    test('respects collectResourceTiming parameter on source', async () => {
         const source = createSource({collectResourceTiming: true});
         source.map = {
             _requestManager: {
@@ -158,6 +158,7 @@ describe('GeoJSONSource.setData', () => {
             });
         };
         source.setData('http://localhost/nonexistent');
+        await sleep(0); // to resolve pending `await`s
         expect(spy).toHaveBeenCalled();
     });
 
@@ -264,7 +265,7 @@ describe('GeoJSONSource.update', () => {
     transform.setZoom(15);
     transform.setLocationAtPoint(lngLat, point);
 
-    test('sends initial loadData request to dispatcher', () => {
+    test('sends initial loadData request to dispatcher', async () => {
         const spy = vi.fn();
         const mockDispatcher = wrapDispatcher({
             sendAsync(message: ActorMessage<MessageType>) {
@@ -275,10 +276,11 @@ describe('GeoJSONSource.update', () => {
         });
 
         new GeoJSONSource('id', {data: {}} as GeoJSONSourceOptions, mockDispatcher, undefined).load();
+        await sleep(); // to resolve pending `await`s
         expect(spy).toHaveBeenCalled();
     });
 
-    test('forwards geojson-vt options with worker request', () => {
+    test('forwards geojson-vt options with worker request', async () => {
         const spy = vi.fn();
         const mockDispatcher = wrapDispatcher({
             sendAsync(message: ActorMessage<any>) {
@@ -303,10 +305,11 @@ describe('GeoJSONSource.update', () => {
             buffer: 16,
             generateId: true
         } as GeoJSONSourceOptions, mockDispatcher, undefined).load();
+        await sleep(0); // to resolve pending `await`s
         expect(spy).toHaveBeenCalled();
     });
 
-    test('forwards Supercluster options with worker request', () => {
+    test('forwards Supercluster options with worker request', async () => {
         const spy = vi.fn();
         const mockDispatcher = wrapDispatcher({
             sendAsync(message) {
@@ -333,6 +336,7 @@ describe('GeoJSONSource.update', () => {
             generateId: true
         } as GeoJSONSourceOptions, mockDispatcher, undefined);
         source.load();
+        await sleep(0); // to resolve pending `await`s
         expect(spy).toHaveBeenCalled();
     });
 
@@ -362,6 +366,7 @@ describe('GeoJSONSource.update', () => {
         spy.mockClear();
 
         source.setClusterOptions({cluster: true, clusterRadius: 80, clusterMaxZoom: 16});
+        await sleep(0); // to resolve pending `await`s
 
         expect(spy).toHaveBeenCalledTimes(1);
         expect(spy.mock.calls[0][0].type).toBe(MessageType.loadData);
@@ -390,7 +395,7 @@ describe('GeoJSONSource.update', () => {
 
         // Wait for initial data to be loaded
         source.load();
-        await waitForEvent(source, 'data', (e: MapSourceDataEvent) => e.sourceDataType === 'metadata');
+        await sleep(0); // to resolve all the pending `await`s
 
         spy.mockClear();
 
@@ -403,7 +408,7 @@ describe('GeoJSONSource.update', () => {
         source.setData(sourceData2);
         source.setClusterOptions({cluster: true, clusterRadius: 80, clusterMaxZoom: 16});
 
-        await waitForEvent(source, 'data', (e: MapSourceDataEvent) => e.sourceDataType === 'metadata');
+        await sleep(0); // to resolve all the pending `await`s
 
         expect(spy).toHaveBeenCalledTimes(2);
         expect(spy.mock.calls[0][0].type).toBe(MessageType.loadData);
@@ -460,7 +465,7 @@ describe('GeoJSONSource.update', () => {
         source.updateData(diff);
         source.setClusterOptions({cluster: true, clusterRadius: 80, clusterMaxZoom: 16});
 
-        await waitForEvent(source, 'data', (e: MapSourceDataEvent) => e.sourceDataType === 'metadata');
+        await sleep(0); // to resolve all the pending `await`s
 
         expect(spy).toHaveBeenCalledTimes(2);
         expect(spy.mock.calls[0][0].data.cluster).toBe(false);
@@ -470,7 +475,7 @@ describe('GeoJSONSource.update', () => {
         expect(spy.mock.calls[1][0].data.dataDiff).not.toBeDefined();
     });
 
-    test('forwards Supercluster options with worker request, ignore max zoom of source', () => {
+    test('forwards Supercluster options with worker request, ignore max zoom of source', async () => {
         const spy = vi.fn();
         vi.spyOn(console, 'warn').mockImplementation(() => {});
         const mockDispatcher = wrapDispatcher({
@@ -499,6 +504,7 @@ describe('GeoJSONSource.update', () => {
             generateId: true
         } as GeoJSONSourceOptions, mockDispatcher, undefined);
         source.load();
+        await sleep(0); // to resolve pending `await`s
         expect(spy).toHaveBeenCalled();
     });
 

src/source/geojson_source.ts

@@ -390,8 +390,8 @@ export class GeoJSONSource extends Evented implements Source {
         }
 
         const {data, diff} = this._pendingWorkerUpdate;
-        const paramsMaybePromise = this._getLoadGeoJSONParameters(data, diff);
-        const params = paramsMaybePromise instanceof Promise ? await paramsMaybePromise : paramsMaybePromise;
+        // delay awaiting params until _isUpdatingWorker is set, otherwise, a race condition can happen
+        const params = this._getLoadGeoJSONParameters(data, diff);
 
         if (data !== undefined) {
             this._pendingWorkerUpdate.data = undefined;
@@ -408,23 +408,14 @@ export class GeoJSONSource extends Evented implements Source {
     /**
      * Create the parameters object that will be sent to the worker and used to load GeoJSON.
      */
-    private _getLoadGeoJSONParameters(data: string | GeoJSON.GeoJSON<GeoJSON.Geometry>, diff: GeoJSONSourceDiff): LoadGeoJSONParameters | Promise<LoadGeoJSONParameters> {
+    private async _getLoadGeoJSONParameters(data: string | GeoJSON.GeoJSON<GeoJSON.Geometry>, diff: GeoJSONSourceDiff): Promise<LoadGeoJSONParameters> {
         const params: LoadGeoJSONParameters = extend({type: this.type}, this.workerOptions);
 
         // Data comes from a remote url
         if (typeof data === 'string') {
-            const request = this.map._requestManager.transformRequest(browser.resolveURL(data as string), ResourceType.Source);
-            if (request instanceof Promise) {
-                return request.then((request) => {
-                    request.collectResourceTiming = this._collectResourceTiming;
-                    params.request = request;
-                    return params;
-                });
-            } else {
-                request.collectResourceTiming = this._collectResourceTiming;
-                params.request = request;
-                return params;
-            }
+            params.request = await this.map._requestManager.transformRequest(browser.resolveURL(data as string), ResourceType.Source);
+            params.request.collectResourceTiming = this._collectResourceTiming;
+            return params;
         }
 
         // Data is a geojson object
@@ -445,11 +436,12 @@ export class GeoJSONSource extends Evented implements Source {
     /**
      * Send the worker update data from the main thread to the worker
      */
-    private async _dispatchWorkerUpdate(options: LoadGeoJSONParameters) {
+    private async _dispatchWorkerUpdate(optionsPromise: Promise<LoadGeoJSONParameters>) {
         this._isUpdatingWorker = true;
         this.fire(new Event('dataloading', {dataType: 'source'}));
 
         try {
+            const options = await optionsPromise;
             const result = await this.actor.sendAsync({type: MessageType.loadData, data: options});
             this._isUpdatingWorker = false;