Fix actor not passing additional callback arguments

CHANGELOG.md

@@ -5,6 +5,7 @@
 ### 🐞 Bug fixes
 
 - _...Add new stuff here..._
+- Fixed actor passing only the first 2 arguments to callbacks ([#3130](https://github.com/maplibre/maplibre-gl-js/pull/3196))
 
 ## 3.5.2
 

src/util/actor.test.ts

@@ -63,6 +63,32 @@ describe('Actor', () => {
         });
     });
 
+    test('calls the callback with the correct number of arguments', done => {
+        setTestWorker(class MockWorker {
+            self: any;
+            actor: Actor;
+            constructor(self) {
+                this.self = self;
+                this.actor = new Actor(self, this);
+            }
+            test(mapId, params, callback) {
+                setTimeout(callback, 0, null, '1st param', '2nd param', '3rd param');
+            }
+        });
+
+        const worker = workerFactory();
+
+        const m = new Actor(worker, {}, '1');
+
+        m.send('test', {value: 1729}, (err, param1, param2, param3) => {
+            expect(err).toBeFalsy();
+            expect(param1).toBe('1st param');
+            expect(param2).toBe('2nd param');
+            expect(param3).toBe('3rd param');
+            done();
+        });
+    });
+
     test('targets worker-initiated messages to correct map instance', done => {
         let workerActor;
 

src/util/actor.ts

@@ -31,26 +31,34 @@ export interface GlyphsProvider {
     );
 }
 
-export type MessageType = '<response>' | '<cancel>' |
-'geojson.getClusterExpansionZoom' | 'geojson.getClusterChildren' | 'geojson.getClusterLeaves' | 'geojson.loadData' |
+export type MessageType = 'geojson.getClusterExpansionZoom' | 'geojson.getClusterChildren' |
+'geojson.getClusterLeaves' | 'geojson.loadData' |
 'removeSource' | 'loadWorkerSource' | 'loadDEMTile' | 'removeDEMTile' |
 'removeTile' | 'reloadTile' | 'abortTile' | 'loadTile' | 'getTile' |
 'getGlyphs' | 'getImages' | 'setImages' |
 'syncRTLPluginState' | 'setReferrer' | 'setLayers' | 'updateLayers';
 
-export type MessageData = {
+type BasicMessage = {
     id: string;
+    targetMapId?: string | null;
+    sourceMapId: string | number | null;
+}
+
+type RequestMessage = BasicMessage & {
     type: MessageType;
-    data?: Serialized;
-    targetMapId?: string | number | null;
-    mustQueue?: boolean;
+    hasCallback: boolean;
+    mustQueue: boolean;
+    params: Serialized;
+}
+
+type ResponseMessage = BasicMessage & {
+    type: '<response>';
     error?: Serialized | null;
-    hasCallback?: boolean;
-    sourceMapId: string | number | null;
+    callbackArgs: Array<Serialized>;
 }
 
-export type Message = {
-    data: MessageData;
+type CancelMessage = BasicMessage & {
+    type: '<cancel>';
 }
 
 /**
@@ -59,15 +67,16 @@ export type Message = {
  * that spin them off - in this case, tasks like parsing parts of styles,
  * owned by the styles
  */
+
 export class Actor {
     target: ActorTarget;
     parent: WorkerSourceProvider | GlyphsProvider;
     mapId: string | number | null;
-    callbacks: { [x: number]: Function};
+    callbacks: Record<string, Function> = {};
     name: string;
-    tasks: { [x: number]: MessageData };
-    taskQueue: Array<string>;
-    cancelCallbacks: { [x: number]: () => void };
+    tasks: Record<string, RequestMessage | ResponseMessage> = {};
+    taskQueue: Array<string> = [];
+    cancelCallbacks: Record<string, Cancelable['cancel']> = {};
     invoker: ThrottledInvoker;
     globalScope: ActorTarget;
 
@@ -80,10 +89,6 @@ export class Actor {
         this.target = target;
         this.parent = parent;
         this.mapId = mapId;
-        this.callbacks = {};
-        this.tasks = {};
-        this.taskQueue = [];
-        this.cancelCallbacks = {};
         this.invoker = new ThrottledInvoker(this.process);
         this.target.addEventListener('message', this.receive, false);
         this.globalScope = isWorker() ? target : window;
@@ -98,7 +103,7 @@ export class Actor {
      */
     send(
         type: MessageType,
-        data: unknown,
+        params: unknown,
         callback?: Function | null,
         targetMapId?: string | null,
         mustQueue: boolean = false
@@ -112,35 +117,34 @@ export class Actor {
             this.callbacks[id] = callback;
         }
         const buffers: Array<Transferable> = [];
-        const message: MessageData = {
+        this.target.postMessage({
             id,
             type,
             hasCallback: !!callback,
             targetMapId,
             mustQueue,
             sourceMapId: this.mapId,
-            data: serialize(data, buffers)
-        };
+            params: serialize(params, buffers)
+        } satisfies RequestMessage, {transfer: buffers});
 
-        this.target.postMessage(message, {transfer: buffers});
         return {
             cancel: () => {
                 if (callback) {
                     // Set the callback to null so that it never fires after the request is aborted.
                     delete this.callbacks[id];
                 }
-                const cancelMessage: MessageData = {
+
+                this.target.postMessage({
                     id,
                     type: '<cancel>',
                     targetMapId,
                     sourceMapId: this.mapId
-                };
-                this.target.postMessage(cancelMessage);
+                } satisfies CancelMessage);
             }
         };
     }
 
-    receive = (message: Message) => {
+    receive = (message: { data: RequestMessage | ResponseMessage | CancelMessage }) => {
         const data = message.data;
         const id = data.id;
 
@@ -163,7 +167,7 @@ export class Actor {
                 cancel();
             }
         } else {
-            if (isWorker() || data.mustQueue) {
+            if (isWorker() || data.type !== '<response>' && data.mustQueue) {
                 // In workers, store the tasks that we need to process before actually processing them. This
                 // is necessary because we want to keep receiving messages, and in particular,
                 // <cancel> messages. Some tasks may take a while in the worker thread, so before
@@ -202,7 +206,7 @@ export class Actor {
         this.processTask(id, task);
     };
 
-    processTask(id: string, task: MessageData) {
+    processTask(id: string, task: RequestMessage | ResponseMessage) {
         if (task.type === '<response>') {
             // The done() function in the counterpart has been called, and we are now
             // firing the callback in the originating actor, if there is one.
@@ -213,29 +217,28 @@ export class Actor {
                 if (task.error) {
                     callback(deserialize(task.error));
                 } else {
-                    callback(null, deserialize(task.data));
+                    callback(null, ...(task.callbackArgs.map(deserialize)));
                 }
             }
         } else {
             let completed = false;
             const buffers: Array<Transferable> = [];
-            const done = task.hasCallback ? (err: Error, data?: any) => {
+            const done = task.hasCallback ? (err: Error, ...callbackArgs: unknown[]) => {
                 completed = true;
                 delete this.cancelCallbacks[id];
-                const responseMessage: MessageData = {
+                this.target.postMessage({
                     id,
                     type: '<response>',
                     sourceMapId: this.mapId,
                     error: err ? serialize(err) : null,
-                    data: serialize(data, buffers)
-                };
-                this.target.postMessage(responseMessage, {transfer: buffers});
+                    callbackArgs: callbackArgs.map(data => serialize(data, buffers))
+                } satisfies ResponseMessage, {transfer: buffers});
             } : (_) => {
                 completed = true;
             };
 
-            let callback: Cancelable = null;
-            const params = deserialize(task.data);
+            let callback: Cancelable | null = null;
+            const params = deserialize(task.params);
             if (this.parent[task.type]) {
                 // task.type == 'loadTile', 'removeTile', etc.
                 callback = this.parent[task.type](task.sourceMapId, params, done);

test/integration/browser/browser.test.ts

@@ -248,4 +248,65 @@ describe('Browser tests', () => {
         expect(markerScreenPosition.x).toBeCloseTo(386.5);
         expect(markerScreenPosition.y).toBeCloseTo(378.1);
     }, 20000);
+
+    test('Expired Tile Reload', async() => {
+        const callCountByUrl = await page.evaluate(() => {
+            const CACHE_SECS = 1;
+            const TILES_URL = 'https://demotiles.maplibre.org/tiles/:z/:x/:y.pbf';
+
+            const callCountByUrl: Record<string, number> = {};
+
+            maplibregl.addProtocol('cust', ({url}, callback) => {
+                const [x, y, z] = url.split('//')[1].split('-');
+
+                callCountByUrl[url] = (callCountByUrl[url] ?? 0) + 1;
+
+                const cacheControl = `no-store,max-age=${CACHE_SECS}`;
+                const expires = new Date(Date.now() + CACHE_SECS * 1000).toUTCString();
+
+                const abortController = new AbortController();
+
+                const tileUrl = TILES_URL.replace(':z', z).replace(':x', x).replace(':y', y);
+
+                fetch(tileUrl, {
+                    signal: abortController.signal,
+                })
+                    .then((response) => response.arrayBuffer())
+                    .then((data) => {
+                        callback(undefined, data, cacheControl, expires);
+                    })
+                    .catch((err) => {
+                        callback(err);
+                    });
+
+                return {
+                    cancel: () => {
+                        abortController.abort();
+                    },
+                };
+            });
+
+            return new Promise<Record<string, number>>((resolve) => {
+                map.addSource('cust', {
+                    type: 'vector',
+                    tiles: ['cust://{x}-{y}-{z}'],
+                });
+
+                map.addLayer({
+                    id: 'cust',
+                    type: 'circle',
+                    source: 'cust',
+                    'source-layer': 'centroids',
+                });
+
+                setTimeout(() => {
+                    resolve(callCountByUrl);
+                }, 5000);
+            });
+        });
+
+        for (const [, count] of  Object.entries(callCountByUrl)) {
+            expect(count).toBeGreaterThan(1);
+        }
+    }, 20000);
 });