Remove callbacks, cancelable, simplify addProtocol, add global dispat…

…cher (#3433) * Remove callbacks, cancelable and simplify add protocol * Update changelog with PR id. * Correct example * Update CHANGELOG.md * Fix issue with RTL dispatcher related to worker code. * Fixes the problem with the dispatcher --------- Co-authored-by: neodescis <nsteinbaugh@gmail.com>
maplibre · Dec 3, 2023 · 3abe643 · 3abe643
1 parent 47d7403
commit 3abe643
Show file tree

Hide file tree

Showing 19 changed files with 173 additions and 143 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 ### ✨ Features and improvements
 
+- ⚠️ Changes `addProtocol` to be promise-based without the usage of callbacks and cancelable ([#3433](https://github.com/maplibre/maplibre-gl-js/pull/3433))
 - ⚠️ Moved the `addSrouceType` to be a part of the global maplibregl object instead of being per map object ([#3420](https://github.com/maplibre/maplibre-gl-js/pull/3420))
 - ⚠️ Removed callback usage from `map.loadImage` in continue to below change ([#3422](https://github.com/maplibre/maplibre-gl-js/pull/3422))
 - ⚠️ Changed the `GeoJSONSource`'s `getClusterExpansionZoom`, `getClusterChildren`, `getClusterLeaves` methods to return a `Promise` instead of a callback usage ([#3421](https://github.com/maplibre/maplibre-gl-js/pull/3421))

diff --git a/src/index.test.ts b/src/index.test.ts
@@ -20,15 +20,15 @@ describe('maplibre', () => {
         const protocolName = 'custom';
         expect(Object.keys(config.REGISTERED_PROTOCOLS)).toHaveLength(0);
 
-        maplibre.addProtocol(protocolName, () => { return {cancel: () => { }}; });
+        maplibre.addProtocol(protocolName, async () => Promise.resolve({} as any));
         expect(Object.keys(config.REGISTERED_PROTOCOLS)[0]).toBe(protocolName);
     });
 
     test('removeProtocol', () => {
         const protocolName = 'custom';
         expect(Object.keys(config.REGISTERED_PROTOCOLS)).toHaveLength(0);
 
-        maplibre.addProtocol(protocolName, () => { return {cancel: () => { }}; });
+        maplibre.addProtocol(protocolName, () => Promise.resolve({} as any));
         expect(Object.keys(config.REGISTERED_PROTOCOLS)[0]).toBe(protocolName);
 
         maplibre.removeProtocol(protocolName);
@@ -37,10 +37,9 @@ describe('maplibre', () => {
 
     test('#addProtocol - getJSON', async () => {
         let protocolCallbackCalled = false;
-        maplibre.addProtocol('custom', (reqParam, callback) => {
+        maplibre.addProtocol('custom', () => {
             protocolCallbackCalled = true;
-            callback(null, {'foo': 'bar'});
-            return {cancel: () => {}};
+            return Promise.resolve({data: {'foo': 'bar'}});
         });
         const response = await getJSON({url: 'custom://test/url/json'}, new AbortController());
         expect(response.data).toEqual({foo: 'bar'});
@@ -49,10 +48,9 @@ describe('maplibre', () => {
 
     test('#addProtocol - getArrayBuffer', async () => {
         let protocolCallbackCalled = false;
-        maplibre.addProtocol('custom', (_reqParam, callback) => {
+        maplibre.addProtocol('custom', () => {
             protocolCallbackCalled = true;
-            callback(null, new ArrayBuffer(1), 'cache-control', 'expires');
-            return {cancel: () => {}};
+            return Promise.resolve({data: new ArrayBuffer(1), cacheControl: 'cache-control', expires: 'expires'});
         });
         const response = await getArrayBuffer({url: 'custom://test/url/getArrayBuffer'}, new AbortController());
         expect(response.data).toBeInstanceOf(ArrayBuffer);
@@ -63,10 +61,9 @@ describe('maplibre', () => {
 
     test('#addProtocol - returning ImageBitmap for getImage', async () => {
         let protocolCallbackCalled = false;
-        maplibre.addProtocol('custom', (_reqParam, callback) => {
+        maplibre.addProtocol('custom', () => {
             protocolCallbackCalled = true;
-            callback(null, new ImageBitmap());
-            return {cancel: () => {}};
+            return Promise.resolve({data: new ImageBitmap()});
         });
 
         const img = await ImageRequest.getImage({url: 'custom://test/url/getImage'}, new AbortController());
@@ -76,21 +73,17 @@ describe('maplibre', () => {
 
     test('#addProtocol - returning HTMLImageElement for getImage', async () => {
         let protocolCallbackCalled = false;
-        maplibre.addProtocol('custom', (reqParam, callback) => {
+        maplibre.addProtocol('custom', () => {
             protocolCallbackCalled = true;
-            callback(null, new Image());
-            return {cancel: () => {}};
+            return Promise.resolve({data: new Image()});
         });
         const img = await ImageRequest.getImage({url: 'custom://test/url/getImage'}, new AbortController());
         expect(img.data).toBeInstanceOf(HTMLImageElement);
         expect(protocolCallbackCalled).toBeTruthy();
     });
 
     test('#addProtocol - error', () => {
-        maplibre.addProtocol('custom', (reqParam, callback) => {
-            callback(new Error('error'));
-            return {cancel: () => { }};
-        });
+        maplibre.addProtocol('custom', () => Promise.reject(new Error('test error')));
 
         getJSON({url: 'custom://test/url/json'}, new AbortController()).catch((error) => {
             expect(error).toBeTruthy();
@@ -99,10 +92,11 @@ describe('maplibre', () => {
 
     test('#addProtocol - Cancel request', async () => {
         let cancelCalled = false;
-        maplibre.addProtocol('custom', () => {
-            return {cancel: () => {
+        maplibre.addProtocol('custom', (_req, abortController) => {
+            abortController.signal.addEventListener('abort', () => {
                 cancelCalled = true;
-            }};
+            });
+            return Promise.resolve({} as any);
         });
         const abortController = new AbortController();
         const promise = getJSON({url: 'custom://test/url/json'}, abortController);

diff --git a/src/index.ts b/src/index.ts
@@ -15,16 +15,14 @@ import {LngLatBounds} from './geo/lng_lat_bounds';
 import Point from '@mapbox/point-geometry';
 import {MercatorCoordinate} from './geo/mercator_coordinate';
 import {Evented} from './util/evented';
-import {config} from './util/config';
+import {AddProtocolAction, config} from './util/config';
 import {Debug} from './util/debug';
 import {isSafari} from './util/util';
 import {rtlMainThreadPluginFactory} from './source/rtl_text_plugin_main_thread';
 import {WorkerPool} from './util/worker_pool';
 import {prewarm, clearPrewarmedResources} from './util/global_worker_pool';
 import {PerformanceUtils} from './util/performance';
 import {AJAXError} from './util/ajax';
-import type {RequestParameters, ResponseCallback} from './util/ajax';
-import type {Cancelable} from './types/cancelable';
 import {GeoJSONSource} from './source/geojson_source';
 import {CanvasSource} from './source/canvas_source';
 import {ImageSource} from './source/image_source';
@@ -191,30 +189,22 @@ class MapLibreGL {
      * @example
      * This will fetch a file using the fetch API (this is obviously a non interesting example...)
      * ```ts
-     * maplibregl.addProtocol('custom', (params, callback) => {
-            fetch(`https://${params.url.split("://")[1]}`)
-                .then(t => {
-                    if (t.status == 200) {
-                        t.arrayBuffer().then(arr => {
-                            callback(null, arr, null, null);
-                        });
-                    } else {
-                        callback(new Error(`Tile fetch error: ${t.statusText}`));
-                    }
-                })
-                .catch(e => {
-                    callback(new Error(e));
-                });
-            return { cancel: () => { } };
+     * maplibregl.addProtocol('custom', async (params, abortController) => {
+            const t = await fetch(`https://${params.url.split("://")[1]}`);
+            if (t.status == 200) {
+                const buffer = await t.arrayBuffer();
+                return {data: buffer}
+            } else {
+                throw new Error(`Tile fetch error: ${t.statusText}`));
+            }
         });
      * // the following is an example of a way to return an error when trying to load a tile
-     * maplibregl.addProtocol('custom2', (params, callback) => {
-     *      callback(new Error('someErrorMessage'));
-     *      return { cancel: () => { } };
+     * maplibregl.addProtocol('custom2', async (params, abortController) => {
+     *      throw new Error('someErrorMessage'));
      * });
      * ```
      */
-    static addProtocol(customProtocol: string, loadFn: (requestParameters: RequestParameters, callback: ResponseCallback<any>) => Cancelable) {
+    static addProtocol(customProtocol: string, loadFn: AddProtocolAction) {
         config.REGISTERED_PROTOCOLS[customProtocol] = loadFn;
     }
 

diff --git a/src/source/image_source.ts b/src/source/image_source.ts
@@ -14,7 +14,6 @@ import type {CanvasSourceSpecification} from './canvas_source';
 import type {Map} from '../ui/map';
 import type {Dispatcher} from '../util/dispatcher';
 import type {Tile} from './tile';
-import type {Callback} from '../types/callback';
 import type {VertexBuffer} from '../gl/vertex_buffer';
 import type {
     ImageSourceSpecification,
@@ -280,7 +279,7 @@ export class ImageSource extends Evented implements Source {
         }
     }
 
-    async loadTile(tile: Tile, callback?: Callback<void>): Promise<void> {
+    async loadTile(tile: Tile): Promise<void> {
         // We have a single tile -- whose coordinates are this.tileID -- that
         // covers the image we want to render.  If that's the one being
         // requested, set it up with the image; otherwise, mark the tile as
@@ -293,9 +292,6 @@ export class ImageSource extends Evented implements Source {
         } else {
             tile.state = 'errored';
         }
-        if (callback) {
-            callback();
-        }
     }
 
     serialize(): ImageSourceSpecification | VideoSourceSpecification | CanvasSourceSpecification {

diff --git a/src/source/rtl_text_plugin_main_thread.test.ts b/src/source/rtl_text_plugin_main_thread.test.ts
@@ -2,6 +2,7 @@ import {FakeServer, fakeServer} from 'nise';
 import {rtlMainThreadPluginFactory} from './rtl_text_plugin_main_thread';
 import {sleep} from '../util/test/util';
 import {browser} from '../util/browser';
+import {Dispatcher} from '../util/dispatcher';
 
 const rtlMainThreadPlugin = rtlMainThreadPluginFactory();
 
@@ -13,7 +14,7 @@ describe('RTLMainThreadPlugin', () => {
         global.fetch = null;
         // Reset the singleton instance before each test
         rtlMainThreadPlugin.clearRTLTextPlugin();
-        broadcastSpy = jest.spyOn(rtlMainThreadPlugin.dispatcher, 'broadcast').mockImplementation(() => { return Promise.resolve({} as any); });
+        broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(() => { return Promise.resolve({} as any); });
     });
 
     afterEach(() => {

diff --git a/src/source/rtl_text_plugin_main_thread.ts b/src/source/rtl_text_plugin_main_thread.ts
@@ -2,13 +2,12 @@ import {getArrayBuffer} from '../util/ajax';
 import {browser} from '../util/browser';
 import {Event, Evented} from '../util/evented';
 import {RTLPluginStatus, PluginState} from './rtl_text_plugin_status';
-import {Dispatcher} from '../util/dispatcher';
-import {getGlobalWorkerPool} from '../util/global_worker_pool';
+import {Dispatcher, getGlobalDispatcher} from '../util/dispatcher';
 
 class RTLMainThreadPlugin extends Evented {
     pluginStatus: RTLPluginStatus = 'unavailable';
     pluginURL: string = null;
-    dispatcher: Dispatcher = new Dispatcher(getGlobalWorkerPool(), 'rtl-text-plugin-dispacher');
+    dispatcher: Dispatcher = getGlobalDispatcher();
     queue: PluginState[] = [];
 
     async _sendPluginStateToWorker() {

diff --git a/src/source/source.ts b/src/source/source.ts
@@ -5,8 +5,7 @@ import {GeoJSONSource} from '../source/geojson_source';
 import {VideoSource} from '../source/video_source';
 import {ImageSource} from '../source/image_source';
 import {CanvasSource} from '../source/canvas_source';
-import {Dispatcher} from '../util/dispatcher';
-import {getGlobalWorkerPool} from '../util/global_worker_pool';
+import {Dispatcher, getGlobalDispatcher} from '../util/dispatcher';
 
 import type {SourceSpecification} from '@maplibre/maplibre-gl-style-spec';
 import type {Event, Evented} from '../util/evented';
@@ -194,7 +193,6 @@ export const addSourceType = async (name: string, SourceType: SourceClass): Prom
     if (!SourceType.workerSourceURL) {
         return;
     }
-    const dispatcher = new Dispatcher(getGlobalWorkerPool(), 'add-custom-source-dispatcher');
+    const dispatcher = getGlobalDispatcher();
     await dispatcher.broadcast('loadWorkerSource', SourceType.workerSourceURL.toString());
-    dispatcher.remove();
 };
diff --git a/src/source/source_cache.test.ts b/src/source/source_cache.test.ts
@@ -9,7 +9,6 @@ import {Event, ErrorEvent, Evented} from '../util/evented';
 import {extend} from '../util/util';
 import {browser} from '../util/browser';
 import {Dispatcher} from '../util/dispatcher';
-import {Callback} from '../types/callback';
 import {TileBounds} from './tile_bounds';
 import {sleep} from '../util/test/util';
 
@@ -22,7 +21,7 @@ class SourceMock extends Evented implements Source {
     type: string;
     tileSize: number;
 
-    constructor(id: string, sourceOptions: any, _dispatcher, eventedParent: Evented) {
+    constructor(id: string, sourceOptions: any, _dispatcher: Dispatcher, eventedParent: Evented) {
         super();
         this.id = id;
         this.minzoom = 0;
@@ -34,18 +33,13 @@ class SourceMock extends Evented implements Source {
             this.hasTile = sourceOptions.hasTile;
         }
     }
-    loadTile(tile: Tile, callback?: Callback<void>): Promise<void> {
+    loadTile(tile: Tile): Promise<void> {
         if (this.sourceOptions.expires) {
             tile.setExpiryData({
                 expires: this.sourceOptions.expires
             });
         }
-        if (callback) {
-            setTimeout(callback, 0);
-        } else {
-            return new Promise(resolve => setTimeout(resolve, 0));
-        }
-
+        return new Promise(resolve => setTimeout(resolve, 0));
     }
     loaded() {
         return true;

diff --git a/src/source/worker.ts b/src/source/worker.ts
@@ -1,4 +1,4 @@
-import {Actor, ActorTarget} from '../util/actor';
+import {Actor, ActorTarget, IActor} from '../util/actor';
 import {StyleLayerIndex} from '../style/style_layer_index';
 import {VectorTileWorkerSource} from './vector_tile_worker_source';
 import {RasterDEMTileWorkerSource} from './raster_dem_tile_worker_source';
@@ -218,7 +218,7 @@ export default class Worker {
         if (!this.workerSources[mapId][sourceType][sourceName]) {
             // use a wrapped actor so that we can attach a target mapId param
             // to any messages invoked by the WorkerSource, this is very important when there are multiple maps
-            const actor = {
+            const actor: IActor = {
                 sendAsync: (message, abortController) => {
                     message.targetMapId = mapId;
                     return this.actor.sendAsync(message, abortController);

diff --git a/src/style/style.ts b/src/style/style.ts
@@ -8,7 +8,7 @@ import {Light} from './light';
 import {LineAtlas} from '../render/line_atlas';
 import {pick, clone, extend, deepEqual, filterObject, mapObject} from '../util/util';
 import {coerceSpriteToArray} from '../util/style';
-import {getJSON, getReferrer, makeRequest} from '../util/ajax';
+import {getJSON, getReferrer} from '../util/ajax';
 import {ResourceType} from '../util/request_manager';
 import {browser} from '../util/browser';
 import {Dispatcher} from '../util/dispatcher';
@@ -40,7 +40,6 @@ import type {Transform} from '../geo/transform';
 import type {StyleImage} from './style_image';
 import type {EvaluationParameters} from './evaluation_parameters';
 import type {Placement} from '../symbol/placement';
-import type {GetResourceResponse, RequestParameters} from '../util/ajax';
 import type {
     LayerSpecification,
     FilterSpecification,
@@ -239,9 +238,6 @@ export class Style extends Evented {
         this.dispatcher.registerMessageHandler('getImages', (mapId, params) => {
             return this.getImages(mapId, params);
         });
-        this.dispatcher.registerMessageHandler('getResource', (mapId, params, abortController) => {
-            return this.getResource(mapId, params, abortController);
-        });
         this.imageManager = new ImageManager();
         this.imageManager.setEventedParent(this);
         this.glyphManager = new GlyphManager(map._requestManager, options.localIdeographFontFamily);
@@ -1594,10 +1590,6 @@ export class Style extends Evented {
         return glypgs;
     }
 
-    getResource(mapId: string | number, params: RequestParameters, abortController: AbortController): Promise<GetResourceResponse<any>> {
-        return makeRequest(params, abortController);
-    }
-
     getGlyphsUrl() {
         return this.stylesheet.glyphs || null;
     }

diff --git a/src/types/callback.ts b/src/types/callback.ts
diff --git a/src/types/cancelable.ts b/src/types/cancelable.ts
diff --git a/src/util/actor.ts b/src/util/actor.ts
@@ -54,7 +54,7 @@ export class Actor implements IActor {
     mapId: string | number | null;
     resolveRejects: { [x: string]: ResolveReject};
     name: string;
-    tasks: { [x: number]: MessageData };
+    tasks: { [x: string]: MessageData };
     taskQueue: Array<string>;
     abortControllers: { [x: number | string]: AbortController };
     invoker: ThrottledInvoker;
@@ -205,7 +205,7 @@ export class Actor implements IActor {
             return;
         }
         if (!this.messageHandlers[task.type]) {
-            this.completeTask(id, new Error(`Could not find a registered handler for ${task.type}`));
+            this.completeTask(id, new Error(`Could not find a registered handler for ${task.type}, map ID: ${this.mapId}, available handlers: ${Object.keys(this.messageHandlers).join(', ')}`));
             return;
         }
         const params = deserialize(task.data) as RequestResponseMessageMap[MessageType][0];