Support for RTL text plugin 0.3.0 (#4860)

HarelM · web-flow · commit a2f1450549c9 · 2024-10-21T09:59:24.000Z
* Allow loading rtl plugin async

* Allow loading rtl plugin async

* Add changelog

* Fix typecheck and code

* Add tests to make sure both 0.2.3 and 0.3.0 are supported

* Improve test name

* Change to use latest version everywhere.

* Revert unrelated changes
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,7 @@
 ### 🐞 Bug fixes
 - Fix text not being hidden behind the globe when overlap mode was set to `always` ([#4802](https://github.com/maplibre/maplibre-gl-js/issues/4802))
 - Fix a single white frame being displayed when the map internally transitions from mercator to globe projection ([#4816](https://github.com/maplibre/maplibre-gl-js/issues/4816))
+- Fix loading of RTL plugin version 0.3.0 ([#4860](https://github.com/maplibre/maplibre-gl-js/pull/4860))
 - _...Add new stuff here..._
 
 ## 5.0.0-pre.2
diff --git a/src/index.ts b/src/index.ts
@@ -60,7 +60,7 @@ export type * from '@maplibre/maplibre-gl-style-spec';
  * rtl text will then be rendered only after the plugin finishes loading.
  * @example
  * ```ts
- * setRTLTextPlugin('https://unpkg.com/@mapbox/mapbox-gl-rtl-text@0.2.3/mapbox-gl-rtl-text.js', false);
+ * setRTLTextPlugin('https://unpkg.com/@mapbox/mapbox-gl-rtl-text@0.3.0/dist/mapbox-gl-rtl-text.js', false);
  * ```
  * @see [Add support for right-to-left scripts](https://maplibre.org/maplibre-gl-js/docs/examples/mapbox-gl-rtl-text/)
  */
diff --git a/src/source/rtl_text_plugin_worker.test.ts b/src/source/rtl_text_plugin_worker.test.ts
@@ -0,0 +1,118 @@
+import {PluginState} from './rtl_text_plugin_status';
+import {rtlWorkerPlugin} from './rtl_text_plugin_worker';
+
+describe('RTLWorkerPlugin', () => {
+    beforeEach(() => {
+        // This is a static class, so we need to reset the properties before each test
+        rtlWorkerPlugin.processStyledBidirectionalText = null;
+        rtlWorkerPlugin.processBidirectionalText = null;
+        rtlWorkerPlugin.applyArabicShaping = null;
+    });
+
+    test('should throw if already parsed', () => {
+        const rtlTextPlugin = {
+            applyArabicShaping: jest.fn(),
+            processBidirectionalText: jest.fn(),
+            processStyledBidirectionalText: jest.fn(),
+        };
+
+        rtlWorkerPlugin.setMethods(rtlTextPlugin);
+        expect(() => {
+            rtlWorkerPlugin.setMethods(rtlTextPlugin);
+        }).toThrow('RTL text plugin already registered.');
+    });
+
+    test('should move RTL plugin from unavailable to deferred', async () => {
+        rtlWorkerPlugin.pluginURL = '';
+        rtlWorkerPlugin.pluginStatus = 'unavailable';
+
+        const mockMessage: PluginState = {
+            pluginURL: 'https://somehost/somescript',
+            pluginStatus: 'deferred'
+        };
+
+        await rtlWorkerPlugin.syncState(mockMessage, jest.fn());
+
+        expect(rtlWorkerPlugin.getRTLTextPluginStatus()).toBe('deferred');
+    });
+
+    test('should not change RTL plugin status if already parsed', async () => {
+        const originalUrl = 'https://somehost/somescript1';
+        rtlWorkerPlugin.pluginURL = originalUrl;
+        rtlWorkerPlugin.pluginStatus = 'loaded';
+        rtlWorkerPlugin.setMethods({
+            applyArabicShaping: jest.fn(),
+            processBidirectionalText: jest.fn(),
+            processStyledBidirectionalText: jest.fn(),
+        });
+        const mockMessage: PluginState = {
+            pluginURL: 'https://somehost/somescript2',
+            pluginStatus: 'loading'
+        };
+
+        const workerResult: PluginState = await await rtlWorkerPlugin.syncState(mockMessage, jest.fn());
+
+        expect(rtlWorkerPlugin.getRTLTextPluginStatus()).toBe('loaded');
+        expect(rtlWorkerPlugin.pluginURL).toBe(originalUrl);
+
+        expect(workerResult.pluginStatus).toBe('loaded');
+        expect(workerResult.pluginURL).toBe(originalUrl);
+    });
+
+    test('should do a full cycle of rtl loading synchronously', async () => {
+        const originalUrl = 'https://somehost/somescript1';
+        const loadScriptsMock = jest.fn().mockImplementation((_) => {
+            rtlWorkerPlugin.setMethods({
+                applyArabicShaping: jest.fn(),
+                processBidirectionalText: jest.fn(),
+                processStyledBidirectionalText: jest.fn(),
+            });
+        });
+
+        const workerResult: PluginState = await rtlWorkerPlugin.syncState({
+            pluginURL: originalUrl,
+            pluginStatus: 'loading'
+        }, loadScriptsMock);
+
+        expect(rtlWorkerPlugin.getRTLTextPluginStatus()).toBe('loaded');
+        expect(rtlWorkerPlugin.pluginURL).toBe(originalUrl);
+        expect(workerResult.pluginStatus).toBe('loaded');
+        expect(workerResult.pluginURL).toBe(originalUrl);
+    });
+
+    test('should do a full cycle of rtl loading asynchronously', async () => {
+        const originalUrl = 'https://somehost/somescript1';
+        const loadScriptsMock = jest.fn().mockImplementation((_) => {
+            setTimeout(() => {
+                rtlWorkerPlugin.setMethods({
+                    applyArabicShaping: jest.fn(),
+                    processBidirectionalText: jest.fn(),
+                    processStyledBidirectionalText: jest.fn(),
+                });
+            }, 10);
+        });
+
+        const workerResult: PluginState = await rtlWorkerPlugin.syncState({
+            pluginURL: originalUrl,
+            pluginStatus: 'loading'
+        }, loadScriptsMock);
+
+        expect(rtlWorkerPlugin.getRTLTextPluginStatus()).toBe('loaded');
+        expect(rtlWorkerPlugin.pluginURL).toBe(originalUrl);
+        expect(workerResult.pluginStatus).toBe('loaded');
+        expect(workerResult.pluginURL).toBe(originalUrl);
+    });
+
+    test('should fail loading on timeout', async () => {
+        const originalUrl = 'https://somehost/somescript1';
+        const loadScriptsMock = jest.fn().mockImplementation(() => {});
+
+        (rtlWorkerPlugin as any).TIMEOUT = 1;
+
+        await expect(rtlWorkerPlugin.syncState({
+            pluginURL: originalUrl,
+            pluginStatus: 'loading'
+        }, loadScriptsMock)
+        ).rejects.toThrow('RTL Text Plugin failed to import scripts from https://somehost/somescript1');
+    });
+});
diff --git a/src/source/rtl_text_plugin_worker.ts b/src/source/rtl_text_plugin_worker.ts
@@ -7,42 +7,81 @@ export interface RTLTextPlugin {
 }
 
 class RTLWorkerPlugin implements RTLTextPlugin {
+    readonly TIMEOUT = 5000;
+
     applyArabicShaping: (a: string) => string = null;
     processBidirectionalText: ((b: string, a: Array<number>) => Array<string>) = null;
     processStyledBidirectionalText: ((c: string, b: Array<number>, a: Array<number>) => Array<[string, Array<number>]>) = null;
     pluginStatus: RTLPluginStatus = 'unavailable';
     pluginURL: string = null;
+    loadScriptResolve: () => void = () => {};
 
-    setState(state: PluginState) {
+    private setState(state: PluginState) {
         this.pluginStatus = state.pluginStatus;
         this.pluginURL = state.pluginURL;
     }
 
-    getState(): PluginState {
+    private getState(): PluginState {
         return {
             pluginStatus: this.pluginStatus,
             pluginURL: this.pluginURL
         };
     }
 
-    setMethods(rtlTextPlugin: RTLTextPlugin) {
+    public setMethods(rtlTextPlugin: RTLTextPlugin) {
+        if (rtlWorkerPlugin.isParsed()) {
+            throw new Error('RTL text plugin already registered.');
+        }
         this.applyArabicShaping = rtlTextPlugin.applyArabicShaping;
         this.processBidirectionalText = rtlTextPlugin.processBidirectionalText;
         this.processStyledBidirectionalText = rtlTextPlugin.processStyledBidirectionalText;
+        this.loadScriptResolve();
     }
 
-    isParsed(): boolean {
+    public isParsed(): boolean {
         return this.applyArabicShaping != null &&
             this.processBidirectionalText != null &&
             this.processStyledBidirectionalText != null;
     }
 
-    getPluginURL(): string {
-        return this.pluginURL;
+    public getRTLTextPluginStatus() {
+        return this.pluginStatus;
     }
 
-    getRTLTextPluginStatus() {
-        return this.pluginStatus;
+    public async syncState(incomingState: PluginState, importScripts: (url: string) => void): Promise<PluginState> {
+        // Parsed plugin cannot be changed, so just return its current state.
+        if (this.isParsed()) {
+            return this.getState();
+        }
+
+        if (incomingState.pluginStatus !== 'loading') {
+            // simply sync and done
+            this.setState(incomingState);
+            return incomingState;
+        }
+        const urlToLoad = incomingState.pluginURL;
+        const loadScriptPromise = new Promise<void>((resolve) => {
+            this.loadScriptResolve = resolve;
+        });
+        importScripts(urlToLoad);
+        const dontWaitForeverTimeoutPromise = new Promise<void>((resolve) => setTimeout(() => resolve(), this.TIMEOUT));
+        await Promise.race([loadScriptPromise, dontWaitForeverTimeoutPromise]);
+        const complete = this.isParsed();
+        if (complete) {
+            const loadedState: PluginState = {
+                pluginStatus: 'loaded',
+                pluginURL: urlToLoad
+            };
+            this.setState(loadedState);
+            return loadedState;
+        }
+
+        // error case
+        this.setState({
+            pluginStatus: 'error',
+            pluginURL: ''
+        });
+        throw new Error(`RTL Text Plugin failed to import scripts from ${urlToLoad}`);
     }
 }
 
diff --git a/src/source/worker.test.ts b/src/source/worker.test.ts
@@ -6,7 +6,6 @@ import {CanonicalTileID, OverscaledTileID} from './tile_id';
 import {WorkerSource, WorkerTileParameters, WorkerTileResult} from './worker_source';
 import {rtlWorkerPlugin} from './rtl_text_plugin_worker';
 import {ActorTarget, IActor} from '../util/actor';
-import {PluginState} from './rtl_text_plugin_status';
 import {MessageType} from '../util/actor_messages';
 
 class WorkerSourceMock implements WorkerSource {
@@ -37,108 +36,22 @@ describe('Worker RTLTextPlugin', () => {
         } as any;
         worker = new Worker(_self);
         global.fetch = null;
-        rtlWorkerPlugin.setMethods({
-            applyArabicShaping: null,
-            processBidirectionalText: null,
-            processStyledBidirectionalText: null
-        });
-        jest.spyOn(rtlWorkerPlugin, 'isParsed').mockImplementation(() => {
-            return false;
-        });
-    });
-
-    test('should not throw and set values in plugin', () => {
-        const rtlTextPlugin = {
-            applyArabicShaping: 'test',
-            processBidirectionalText: 'test',
-            processStyledBidirectionalText: 'test',
-        };
-
-        _self.registerRTLTextPlugin(rtlTextPlugin);
-        expect(rtlWorkerPlugin.applyArabicShaping).toBe('test');
-        expect(rtlWorkerPlugin.processBidirectionalText).toBe('test');
-        expect(rtlWorkerPlugin.processStyledBidirectionalText).toBe('test');
-    });
-
-    test('should throw if already parsed', () => {
-        jest.spyOn(rtlWorkerPlugin, 'isParsed').mockImplementation(() => {
-            return true;
-        });
-
-        const rtlTextPlugin = {
-            applyArabicShaping: jest.fn(),
-            processBidirectionalText: jest.fn(),
-            processStyledBidirectionalText: jest.fn(),
-        };
-
-        expect(() => {
-            _self.registerRTLTextPlugin(rtlTextPlugin);
-        }).toThrow('RTL text plugin already registered.');
     });
 
-    test('should move RTL plugin from unavailable to deferred', async () => {
-        rtlWorkerPlugin.setState({
-            pluginURL: '',
-            pluginStatus: 'unavailable'
-        }
-        );
-        const mockMessage: PluginState = {
-            pluginURL: 'https://somehost/somescript',
-            pluginStatus: 'deferred'
-        };
-
-        await worker.actor.messageHandlers[MessageType.syncRTLPluginState]('', mockMessage);
-        expect(rtlWorkerPlugin.getRTLTextPluginStatus()).toBe('deferred');
-    });
+    test('should call setMethods in plugin', () => {
+        const spy = jest.spyOn(rtlWorkerPlugin, 'setMethods').mockImplementation(() => {});
 
-    test('should download RTL plugin when "loading" message is received', async () => {
-        rtlWorkerPlugin.setState({
-            pluginURL: '',
-            pluginStatus: 'deferred'
-        });
+        _self.registerRTLTextPlugin({} as any);
 
-        const mockURL = 'https://somehost/somescript';
-        const mockMessage: PluginState = {
-            pluginURL: mockURL,
-            pluginStatus: 'loading'
-        };
-
-        const importSpy = jest.spyOn(worker.self, 'importScripts').mockImplementation(() => {
-            // after importing isParse() to return true
-            jest.spyOn(rtlWorkerPlugin, 'isParsed').mockImplementation(() => {
-                return true;
-            });
-        });
-
-        const syncResult: PluginState = await worker.actor.messageHandlers[MessageType.syncRTLPluginState]('', mockMessage) as any;
-        expect(rtlWorkerPlugin.getRTLTextPluginStatus()).toBe('loaded');
-        expect(importSpy).toHaveBeenCalledWith(mockURL);
-
-        expect(syncResult.pluginURL).toBe(mockURL);
-        expect(syncResult.pluginStatus).toBe('loaded');
+        expect(spy).toHaveBeenCalled();
     });
 
-    test('should not change RTL plugin status if already parsed', async () => {
-        const originalUrl = 'https://somehost/somescript1';
-        rtlWorkerPlugin.setState({
-            pluginURL: originalUrl,
-            pluginStatus: 'loaded'
-        });
-
-        jest.spyOn(rtlWorkerPlugin, 'isParsed').mockImplementation(() => {
-            return true;
-        });
-        const mockMessage: PluginState = {
-            pluginURL: 'https://somehost/somescript2',
-            pluginStatus: 'loading'
-        };
+    test('should call syncState when rtl message is received', async () => {
+        const syncStateSpy = jest.spyOn(rtlWorkerPlugin, 'syncState').mockImplementation((_, __) => Promise.resolve({} as any));
 
-        const workerResult: PluginState = await worker.actor.messageHandlers[MessageType.syncRTLPluginState]('', mockMessage) as any;
-        expect(rtlWorkerPlugin.getRTLTextPluginStatus()).toBe('loaded');
-        expect(rtlWorkerPlugin.getPluginURL()).toBe(originalUrl);
+        await worker.actor.messageHandlers[MessageType.syncRTLPluginState]('', {} as any) as any;
 
-        expect(workerResult.pluginStatus).toBe('loaded');
-        expect(workerResult.pluginURL).toBe(originalUrl);
+        expect(syncStateSpy).toHaveBeenCalled();
     });
 });
 
diff --git a/src/source/worker.ts b/src/source/worker.ts
@@ -83,9 +83,7 @@ export default class Worker {
 
         // This is invoked by the RTL text plugin when the download via the `importScripts` call has finished, and the code has been parsed.
         this.self.registerRTLTextPlugin = (rtlTextPlugin: RTLTextPlugin) => {
-            if (rtlWorkerPlugin.isParsed()) {
-                throw new Error('RTL text plugin already registered.');
-            }
+
             rtlWorkerPlugin.setMethods(rtlTextPlugin);
         };
 
@@ -191,35 +189,8 @@ export default class Worker {
     }
 
     private async _syncRTLPluginState(mapId: string, incomingState: PluginState): Promise<PluginState> {
-
-        // Parsed plugin cannot be changed, so just return its current state.
-        if (rtlWorkerPlugin.isParsed()) {
-            return rtlWorkerPlugin.getState();
-        }
-
-        if (incomingState.pluginStatus !== 'loading') {
-            // simply sync and done
-            rtlWorkerPlugin.setState(incomingState);
-            return incomingState;
-        }
-        const urlToLoad = incomingState.pluginURL;
-        this.self.importScripts(urlToLoad);
-        const complete = rtlWorkerPlugin.isParsed();
-        if (complete) {
-            const loadedState: PluginState = {
-                pluginStatus: 'loaded',
-                pluginURL: urlToLoad
-            };
-            rtlWorkerPlugin.setState(loadedState);
-            return loadedState;
-        }
-
-        // error case
-        rtlWorkerPlugin.setState({
-            pluginStatus: 'error',
-            pluginURL: ''
-        });
-        throw new Error(`RTL Text Plugin failed to import scripts from ${urlToLoad}`);
+        const state = await rtlWorkerPlugin.syncState(incomingState, this.self.importScripts);
+        return state;
     }
 
     private _getAvailableImages(mapId: string) {
diff --git a/test/examples/display-and-style-rich-text-labels.html b/test/examples/display-and-style-rich-text-labels.html
diff --git a/test/examples/mapbox-gl-rtl-text.html b/test/examples/mapbox-gl-rtl-text.html
diff --git a/test/integration/render/run_render_tests.ts b/test/integration/render/run_render_tests.ts
