From 251751ed505bd7668abb3dbb4abe26ec66cb6108 Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Tue, 20 Feb 2024 11:31:03 -0800 Subject: [PATCH 01/30] fire pluginStateChange only if at least one receiver changed plugin status --- src/source/rtl_text_plugin_main_thread.ts | 18 ++++++++++++++++-- src/source/worker.ts | 7 ++++++- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/src/source/rtl_text_plugin_main_thread.ts b/src/source/rtl_text_plugin_main_thread.ts index 8cecd06477..82215cd886 100644 --- a/src/source/rtl_text_plugin_main_thread.ts +++ b/src/source/rtl_text_plugin_main_thread.ts @@ -10,9 +10,23 @@ class RTLMainThreadPlugin extends Evented { dispatcher: Dispatcher = getGlobalDispatcher(); queue: PluginState[] = []; - async _sendPluginStateToWorker() { + private async _sendPluginStateToWorker() { await this.dispatcher.broadcast('syncRTLPluginState', {pluginStatus: this.pluginStatus, pluginURL: this.pluginURL}); - this.fire(new Event('pluginStateChange', {pluginStatus: this.pluginStatus, pluginURL: this.pluginURL})); + this.dispatcher.broadcast('syncRTLPluginState', {pluginStatus: this.pluginStatus, pluginURL: this.pluginURL}).then( + (broadCastResults: boolean[]) => { + // should fire pluginStateChange event only if at least one of the receiver actually changed plugin status + if (broadCastResults) { + for (const result of broadCastResults) { + if (result) { + this.fire(new Event('pluginStateChange', + { + pluginStatus: this.pluginStatus, + pluginURL: this.pluginURL + })); + } + } + } + }); } getRTLTextPluginStatus() { diff --git a/src/source/worker.ts b/src/source/worker.ts index f75120159c..04f9d229d4 100644 --- a/src/source/worker.ts +++ b/src/source/worker.ts @@ -177,7 +177,12 @@ export default class Worker { private async _syncRTLPluginState(map: string, state: PluginState): Promise { rtlWorkerPlugin.setState(state); const pluginURL = rtlWorkerPlugin.getPluginURL(); - if (state.pluginStatus === 'loaded' && !rtlWorkerPlugin.isParsed() && pluginURL != null) { + + const isLoaded = + state.pluginStatus === 'loaded' || // Main Thread: loaded if the completion callback returned successfully + rtlWorkerPlugin.applyArabicShaping != null; // Web-worker: loaded if the plugin functions have been compiled + + if (isLoaded && !rtlWorkerPlugin.isParsed() && pluginURL != null) { this.self.importScripts(pluginURL); const complete = rtlWorkerPlugin.isParsed(); if (complete) { From 4009780be73205135c653b830568d9bc848b2251 Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Mon, 26 Feb 2024 16:30:07 -0800 Subject: [PATCH 02/30] draft --- .../rtl_text_plugin_main_thread.test.ts | 16 +-- src/source/rtl_text_plugin_main_thread.ts | 111 ++++++++++-------- src/source/rtl_text_plugin_status.ts | 18 ++- src/source/tile.ts | 2 +- src/source/worker.ts | 31 +++-- src/util/actor_messages.ts | 4 +- 6 files changed, 103 insertions(+), 79 deletions(-) diff --git a/src/source/rtl_text_plugin_main_thread.test.ts b/src/source/rtl_text_plugin_main_thread.test.ts index e54c5a1355..2f2b9999ad 100644 --- a/src/source/rtl_text_plugin_main_thread.test.ts +++ b/src/source/rtl_text_plugin_main_thread.test.ts @@ -35,14 +35,14 @@ describe('RTLMainThreadPlugin', () => { await sleep(0); server.respond(); await promise; - expect(rtlMainThreadPlugin.pluginURL).toEqual(url); + expect(rtlMainThreadPlugin.url).toEqual(url); }); it('should set the RTL text plugin but deffer downloading', async () => { const url = 'http://example.com/plugin'; await rtlMainThreadPlugin.setRTLTextPlugin(url, true); expect(server.requests).toHaveLength(0); - expect(rtlMainThreadPlugin.pluginStatus).toBe('deferred'); + expect(rtlMainThreadPlugin.status).toBe('deferred'); }); it('should throw if the plugin is already set', async () => { @@ -64,20 +64,20 @@ describe('RTLMainThreadPlugin', () => { await sleep(0); server.respond(); await promise; - expect(rtlMainThreadPlugin.pluginURL).toEqual(url); - expect(rtlMainThreadPlugin.pluginStatus).toBe('error'); + expect(rtlMainThreadPlugin.url).toEqual(url); + expect(rtlMainThreadPlugin.status).toBe('error'); }); - it('should lazy load the plugin if deffered', async () => { + it('should lazy load the plugin if deferred', async () => { const url = 'http://example.com/plugin'; server.respondWith(new ArrayBuffer(0)); await rtlMainThreadPlugin.setRTLTextPlugin(url, true); expect(server.requests).toHaveLength(0); - expect(rtlMainThreadPlugin.pluginStatus).toBe('deferred'); - const promise = rtlMainThreadPlugin.lazyLoadRTLTextPlugin(); + expect(rtlMainThreadPlugin.status).toBe('deferred'); + const promise = rtlMainThreadPlugin.lazyLoad(); await sleep(0); server.respond(); await promise; - expect(rtlMainThreadPlugin.pluginStatus).toBe('loaded'); + expect(rtlMainThreadPlugin.status).toBe('loaded'); }); }); diff --git a/src/source/rtl_text_plugin_main_thread.ts b/src/source/rtl_text_plugin_main_thread.ts index 82215cd886..b3e8df01b5 100644 --- a/src/source/rtl_text_plugin_main_thread.ts +++ b/src/source/rtl_text_plugin_main_thread.ts @@ -1,74 +1,85 @@ -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 {RTLPluginStatus, RTLPluginLoadedEventName, SyncRTLPluginStateMessageName} from './rtl_text_plugin_status'; import {Dispatcher, getGlobalDispatcher} from '../util/dispatcher'; class RTLMainThreadPlugin extends Evented { - pluginStatus: RTLPluginStatus = 'unavailable'; - pluginURL: string = null; + status: RTLPluginStatus = 'unavailable'; + url: string = null; dispatcher: Dispatcher = getGlobalDispatcher(); - queue: PluginState[] = []; - private async _sendPluginStateToWorker() { - await this.dispatcher.broadcast('syncRTLPluginState', {pluginStatus: this.pluginStatus, pluginURL: this.pluginURL}); - this.dispatcher.broadcast('syncRTLPluginState', {pluginStatus: this.pluginStatus, pluginURL: this.pluginURL}).then( - (broadCastResults: boolean[]) => { - // should fire pluginStateChange event only if at least one of the receiver actually changed plugin status - if (broadCastResults) { - for (const result of broadCastResults) { - if (result) { - this.fire(new Event('pluginStateChange', - { - pluginStatus: this.pluginStatus, - pluginURL: this.pluginURL - })); - } - } + /** Download RTL plugin by sending a message to worker and process its response */ + async _download() : Promise { + this.status = 'loading'; + const workerResults = await this.dispatcher.broadcast( + SyncRTLPluginStateMessageName, + { + pluginStatus: 'loading', + pluginURL: this.url + } + ); + + if (workerResults.length > 0) { + const workerResult = workerResults[0]; + this.status = workerResult.pluginStatus; + if (workerResult.pluginStatus === 'loaded') { + // success scenario + this.fire(new Event(RTLPluginLoadedEventName)); + } else { + // failed scenario: returned, but status is not loaded. + if (workerResult.error) { + throw workerResult.error; + } else { + throw new Error(`worker failed '${SyncRTLPluginStateMessageName}' for unknown reason`); } - }); + } + } else { + // failed scenario(edge case): worker did not respond + this.status = 'error'; + throw new Error(`worker did not respond to message: ${SyncRTLPluginStateMessageName}`); + } } - getRTLTextPluginStatus() { - return this.pluginStatus; + getRTLTextPluginStatus(): RTLPluginStatus { + return this.status; } - clearRTLTextPlugin() { - this.pluginStatus = 'unavailable'; - this.pluginURL = null; + clearRTLTextPlugin(): void { + this.status = 'unavailable'; + this.url = null; } async setRTLTextPlugin(url: string, deferred: boolean = false): Promise { - if (this.pluginStatus === 'deferred' || this.pluginStatus === 'loading' || this.pluginStatus === 'loaded') { + if (this.url) { + // error throw new Error('setRTLTextPlugin cannot be called multiple times.'); } - this.pluginURL = browser.resolveURL(url); - this.pluginStatus = 'deferred'; - await this._sendPluginStateToWorker(); - if (!deferred) { - //Start downloading the plugin immediately if not intending to lazy-load - await this._downloadRTLTextPlugin(); - } - } - async _downloadRTLTextPlugin() { - if (this.pluginStatus !== 'deferred' || !this.pluginURL) { - throw new Error('rtl-text-plugin cannot be downloaded unless a pluginURL is specified'); - } - try { - this.pluginStatus = 'loading'; - await this._sendPluginStateToWorker(); - await getArrayBuffer({url: this.pluginURL}, new AbortController()); - this.pluginStatus = 'loaded'; - } catch { - this.pluginStatus = 'error'; + this.url = browser.resolveURL(url); + if (this.status === 'unavailable') { + + // from initial state: + if (deferred) { + // nothing else to do, just wait + this.status = 'deferred'; + } else { + // immediate download + return this._download(); + } + + } else if (this.status === 'requested') { + // already requested, start downloading + return this._download(); } - await this._sendPluginStateToWorker(); } - async lazyLoadRTLTextPlugin() { - if (this.pluginStatus === 'deferred') { - await this._downloadRTLTextPlugin(); + /** Start a lazy loading process of RTL plugin */ + lazyLoad(): void { + if (this.status === 'unavailable') { + this.status = 'requested'; + } else if (this.status === 'deferred') { + this._download(); } } } diff --git a/src/source/rtl_text_plugin_status.ts b/src/source/rtl_text_plugin_status.ts index 44abc6f3b7..04a69858f3 100644 --- a/src/source/rtl_text_plugin_status.ts +++ b/src/source/rtl_text_plugin_status.ts @@ -5,13 +5,21 @@ * * `deferred`: The plugin URL has been specified, but loading has been deferred. * - * `loading`: request in-flight. + * `requested`: at least one tile needs RTL to render, but the plugin has not been set + * + * `loading`: RTL is in the process of being loaded by worker. * * `loaded`: The plugin is now loaded * * `error`: The plugin failed to load */ -export type RTLPluginStatus = 'unavailable' | 'deferred' | 'loading' | 'loaded' | 'error'; +export type RTLPluginStatus = + 'unavailable' | + 'deferred' | + 'requested' | + 'loading' | + 'loaded' | + 'error'; /** * The RTL plugin state @@ -19,4 +27,10 @@ export type RTLPluginStatus = 'unavailable' | 'deferred' | 'loading' | 'loaded' export type PluginState = { pluginStatus: RTLPluginStatus; pluginURL: string; + + /** Optional error object that carries error from worker to main thread */ + error?: Error; }; + +export const RTLPluginLoadedEventName = 'RTLPluginLoaded'; +export const SyncRTLPluginStateMessageName = 'syncRTLPluginState'; diff --git a/src/source/tile.ts b/src/source/tile.ts index 4cf5dbadb7..42e7e8797c 100644 --- a/src/source/tile.ts +++ b/src/source/tile.ts @@ -200,7 +200,7 @@ export class Tile { if (bucket instanceof SymbolBucket) { if (bucket.hasRTLText) { this.hasRTLText = true; - rtlMainThreadPluginFactory().lazyLoadRTLTextPlugin(); + rtlMainThreadPluginFactory().lazyLoad(); break; } } diff --git a/src/source/worker.ts b/src/source/worker.ts index 04f9d229d4..312ff0d972 100644 --- a/src/source/worker.ts +++ b/src/source/worker.ts @@ -6,7 +6,7 @@ import {rtlWorkerPlugin, RTLTextPlugin} from './rtl_text_plugin_worker'; import {GeoJSONWorkerSource, LoadGeoJSONParameters} from './geojson_worker_source'; import {isWorker} from '../util/util'; import {addProtocol, removeProtocol} from './protocol_crud'; - +import {RTLPluginStatus, SyncRTLPluginStateMessageName} from './rtl_text_plugin_status'; import type { WorkerSource, WorkerSourceConstructor, @@ -143,7 +143,7 @@ export default class Worker { this.referrer = params; }); - this.actor.registerMessageHandler('syncRTLPluginState', (mapId: string, params: PluginState) => { + this.actor.registerMessageHandler(SyncRTLPluginStateMessageName, (mapId: string, params: PluginState) => { return this._syncRTLPluginState(mapId, params); }); @@ -174,23 +174,22 @@ export default class Worker { } } - private async _syncRTLPluginState(map: string, state: PluginState): Promise { - rtlWorkerPlugin.setState(state); - const pluginURL = rtlWorkerPlugin.getPluginURL(); - - const isLoaded = - state.pluginStatus === 'loaded' || // Main Thread: loaded if the completion callback returned successfully - rtlWorkerPlugin.applyArabicShaping != null; // Web-worker: loaded if the plugin functions have been compiled + private async _syncRTLPluginState(mapId: string, incomingState: PluginState): Promise { + const resultState: PluginState = { + pluginStatus: rtlWorkerPlugin.pluginStatus, + pluginURL: rtlWorkerPlugin.pluginURL + }; - if (isLoaded && !rtlWorkerPlugin.isParsed() && pluginURL != null) { - this.self.importScripts(pluginURL); - const complete = rtlWorkerPlugin.isParsed(); - if (complete) { - return complete; + if (incomingState.pluginStatus === 'loading' && !rtlWorkerPlugin.isParsed() && incomingState.pluginURL != null) { + this.self.importScripts(incomingState.pluginURL); + if (rtlWorkerPlugin.isParsed()) { + resultState.pluginStatus = 'loaded'; + resultState.pluginURL = incomingState.pluginURL; + rtlWorkerPlugin.setState(resultState); } - throw new Error(`RTL Text Plugin failed to import scripts from ${pluginURL}`); } - return false; + + return resultState; } private _getAvailableImages(mapId: string) { diff --git a/src/util/actor_messages.ts b/src/util/actor_messages.ts index 299f74dccd..81e096585b 100644 --- a/src/util/actor_messages.ts +++ b/src/util/actor_messages.ts @@ -3,7 +3,7 @@ import type {TileParameters, WorkerDEMTileParameters, WorkerTileParameters, Work import type {DEMData} from '../data/dem_data'; import type {StyleImage} from '../style/style_image'; import type {StyleGlyph} from '../style/style_glyph'; -import type {PluginState} from '../source/rtl_text_plugin_status'; +import type {PluginState, SyncRTLPluginStateMessageName} from '../source/rtl_text_plugin_status'; import type {LayerSpecification} from '@maplibre/maplibre-gl-style-spec'; import type {OverscaledTileID} from '../source/tile_id'; import type {GetResourceResponse, RequestParameters} from './ajax'; @@ -97,7 +97,7 @@ export type RequestResponseMessageMap = { 'setImages': [string[], void]; 'setLayers': [Array, void]; 'updateLayers': [UpdateLayersParamaeters, void]; - 'syncRTLPluginState': [PluginState, boolean]; + [SyncRTLPluginStateMessageName]: [PluginState, PluginState]; 'setReferrer': [string, void]; 'removeSource': [RemoveSourceParams, void]; 'importScript': [string, void]; From 322317e6abb33193f76abdc626f69fd5e13d1a87 Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Fri, 1 Mar 2024 10:29:11 -0800 Subject: [PATCH 03/30] fixes --- src/data/bucket/symbol_bucket.ts | 18 ++++-- src/index.ts | 8 ++- src/source/rtl_text_plugin_main_thread.ts | 69 ++++++++++++----------- src/source/worker_tile.ts | 6 +- src/util/ajax.ts | 11 +++- 5 files changed, 67 insertions(+), 45 deletions(-) diff --git a/src/data/bucket/symbol_bucket.ts b/src/data/bucket/symbol_bucket.ts index dcbf020c0f..545c16d7ac 100644 --- a/src/data/bucket/symbol_bucket.ts +++ b/src/data/bucket/symbol_bucket.ts @@ -417,7 +417,13 @@ export class SymbolBucket implements Bucket { this.textAnchorOffsets = new TextAnchorOffsetArray(); } - calculateGlyphDependencies(text: string, stack: {[_: number]: boolean}, textAlongLine: boolean, allowVerticalPlacement: boolean, doesAllowVerticalWritingMode: boolean) { + private calculateGlyphDependencies( + text: string, + stack: {[_: number]: boolean}, + textAlongLine: boolean, + allowVerticalPlacement: boolean, + doesAllowVerticalWritingMode: boolean) { + for (let i = 0; i < text.length; i++) { stack[text.charCodeAt(i)] = true; if ((textAlongLine || allowVerticalPlacement) && doesAllowVerticalWritingMode) { @@ -476,13 +482,13 @@ export class SymbolBucket implements Bucket { // conversion here. const resolvedTokens = layer.getValueAndResolveTokens('text-field', evaluationFeature, canonical, availableImages); const formattedText = Formatted.factory(resolvedTokens); - if (containsRTLText(formattedText)) { - this.hasRTLText = true; - } + + // hasRTLText only needs to be checked once per bucket + const bucketHasRTLText = this.hasRTLText = (this.hasRTLText || containsRTLText(formattedText)); if ( - !this.hasRTLText || // non-rtl text so can proceed safely + !bucketHasRTLText || // non-rtl text so can proceed safely rtlWorkerPlugin.getRTLTextPluginStatus() === 'unavailable' || // We don't intend to lazy-load the rtl text plugin, so proceed with incorrect shaping - this.hasRTLText && rtlWorkerPlugin.isParsed() // Use the rtlText plugin to shape text + bucketHasRTLText && rtlWorkerPlugin.isParsed() // Use the rtlText plugin to shape text ) { text = transformText(formattedText, layer, evaluationFeature); } diff --git a/src/index.ts b/src/index.ts index 950d569be1..2c30b269ff 100644 --- a/src/index.ts +++ b/src/index.ts @@ -62,7 +62,9 @@ export type * from '@maplibre/maplibre-gl-style-spec'; * ``` * @see [Add support for right-to-left scripts](https://maplibre.org/maplibre-gl-js/docs/examples/mapbox-gl-rtl-text/) */ -function setRTLTextPlugin(pluginURL: string, lazy: boolean) { return rtlMainThreadPluginFactory().setRTLTextPlugin(pluginURL, lazy); } +function setRTLTextPlugin(pluginURL: string, lazy: boolean): Promise { + return rtlMainThreadPluginFactory().setRTLTextPlugin(pluginURL, lazy); +} /** * Gets the map's [RTL text plugin](https://www.mapbox.com/mapbox-gl-js/plugins/#mapbox-gl-rtl-text) status. * The status can be `unavailable` (i.e. not requested or removed), `loading`, `loaded` or `error`. @@ -73,7 +75,9 @@ function setRTLTextPlugin(pluginURL: string, lazy: boolean) { return rtlMainThre * const pluginStatus = getRTLTextPluginStatus(); * ``` */ -function getRTLTextPluginStatus() { return rtlMainThreadPluginFactory().getRTLTextPluginStatus(); } +function getRTLTextPluginStatus(): string { + return rtlMainThreadPluginFactory().getRTLTextPluginStatus(); +} /** * Returns the package version of the library * @returns Package version of the library diff --git a/src/source/rtl_text_plugin_main_thread.ts b/src/source/rtl_text_plugin_main_thread.ts index b3e8df01b5..44198c314c 100644 --- a/src/source/rtl_text_plugin_main_thread.ts +++ b/src/source/rtl_text_plugin_main_thread.ts @@ -1,7 +1,7 @@ import {browser} from '../util/browser'; import {Event, Evented} from '../util/evented'; -import {RTLPluginStatus, RTLPluginLoadedEventName, SyncRTLPluginStateMessageName} from './rtl_text_plugin_status'; +import {RTLPluginStatus, RTLPluginLoadedEventName, SyncRTLPluginStateMessageName, PluginState} from './rtl_text_plugin_status'; import {Dispatcher, getGlobalDispatcher} from '../util/dispatcher'; class RTLMainThreadPlugin extends Evented { @@ -9,38 +9,13 @@ class RTLMainThreadPlugin extends Evented { url: string = null; dispatcher: Dispatcher = getGlobalDispatcher(); - /** Download RTL plugin by sending a message to worker and process its response */ - async _download() : Promise { - this.status = 'loading'; - const workerResults = await this.dispatcher.broadcast( - SyncRTLPluginStateMessageName, - { - pluginStatus: 'loading', - pluginURL: this.url - } - ); - - if (workerResults.length > 0) { - const workerResult = workerResults[0]; - this.status = workerResult.pluginStatus; - if (workerResult.pluginStatus === 'loaded') { - // success scenario - this.fire(new Event(RTLPluginLoadedEventName)); - } else { - // failed scenario: returned, but status is not loaded. - if (workerResult.error) { - throw workerResult.error; - } else { - throw new Error(`worker failed '${SyncRTLPluginStateMessageName}' for unknown reason`); - } - } - } else { - // failed scenario(edge case): worker did not respond - this.status = 'error'; - throw new Error(`worker did not respond to message: ${SyncRTLPluginStateMessageName}`); - } + /** Sync RTL plugin state by broadcasting a message to the worker */ + _syncState(statusToSend: RTLPluginStatus): Promise { + this.status = statusToSend; + return this.dispatcher.broadcast(SyncRTLPluginStateMessageName, {pluginStatus: statusToSend, pluginURL: this.url}); } + /** This one is exposed to outside */ getRTLTextPluginStatus(): RTLPluginStatus { return this.status; } @@ -61,8 +36,13 @@ class RTLMainThreadPlugin extends Evented { // from initial state: if (deferred) { - // nothing else to do, just wait + this.status = 'deferred'; + // fire and forget: in this case it does not need wait for the broadcasting result + // it is important to sync the deferred status once because + // symbol_bucket will be checking it in worker + this._syncState(this.status); + } else { // immediate download return this._download(); @@ -74,6 +54,31 @@ class RTLMainThreadPlugin extends Evented { } } + /** Download RTL plugin by sending a message to worker and process its response */ + async _download() : Promise { + const workerResults = await this._syncState('loading'); + if (workerResults.length > 0) { + const workerResult = workerResults[0]; + this.status = workerResult.pluginStatus; + + // expect worker to return 'loaded' + if (workerResult.pluginStatus === 'loaded') { + this.fire(new Event(RTLPluginLoadedEventName)); + } else { + // failed scenario: returned, but bad status + if (workerResult.error) { + throw workerResult.error; + } else { + throw new Error(`worker failed '${SyncRTLPluginStateMessageName}' for unknown reason`); + } + } + } else { + // failed scenario(edge case): worker did not respond + this.status = 'error'; + throw new Error(`worker did not respond to message: ${SyncRTLPluginStateMessageName}`); + } + } + /** Start a lazy loading process of RTL plugin */ lazyLoad(): void { if (this.status === 'unavailable') { diff --git a/src/source/worker_tile.ts b/src/source/worker_tile.ts index 35c8770cc4..b8e3045100 100644 --- a/src/source/worker_tile.ts +++ b/src/source/worker_tile.ts @@ -112,7 +112,7 @@ export class WorkerTile { recalculateLayers(family, this.zoom, availableImages); - const bucket = buckets[layer.id] = layer.createBucket({ + const bucket: Bucket = buckets[layer.id] = layer.createBucket({ index: featureIndex.bucketLayerIDs.length, layers: family, zoom: this.zoom, @@ -128,7 +128,9 @@ export class WorkerTile { } } - const stacks = mapObject(options.glyphDependencies, (glyphs) => Object.keys(glyphs).map(Number)); + // options.glyphDependencies looks like: {"SomeFontName":{"10":true,"32":true}} + // this line makes an object like: {"SomeFontName":[10,32]} + const stacks: {[_: string]: Array} = mapObject(options.glyphDependencies, (glyphs) => Object.keys(glyphs).map(Number)); this.inFlightDependencies.forEach((request) => request?.abort()); this.inFlightDependencies = []; diff --git a/src/util/ajax.ts b/src/util/ajax.ts index 5690beb3f3..d558b37602 100644 --- a/src/util/ajax.ts +++ b/src/util/ajax.ts @@ -158,9 +158,14 @@ async function makeFetchRequest(requestParameters: RequestParameters, abortContr const body = await response.blob(); throw new AJAXError(response.status, response.statusText, requestParameters.url, body); } - const parsePromise = (requestParameters.type === 'arrayBuffer' || requestParameters.type === 'image') ? response.arrayBuffer() : - requestParameters.type === 'json' ? response.json() : - response.text(); + let parsePromise: Promise; + if ((requestParameters.type === 'arrayBuffer' || requestParameters.type === 'image')) { + parsePromise = response.arrayBuffer(); + } else if (requestParameters.type === 'json') { + parsePromise = response.json(); + } else { + parsePromise = response.text(); + } const result = await parsePromise; if (abortController.signal.aborted) { throw createAbortError(); From e8d23e4d81fac881f06b0339dceb76e080318a38 Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Fri, 1 Mar 2024 12:24:33 -0800 Subject: [PATCH 04/30] all ut pass --- .../rtl_text_plugin_main_thread.test.ts | 67 ++++++++++++------- src/source/rtl_text_plugin_main_thread.ts | 7 +- 2 files changed, 48 insertions(+), 26 deletions(-) diff --git a/src/source/rtl_text_plugin_main_thread.test.ts b/src/source/rtl_text_plugin_main_thread.test.ts index 2f2b9999ad..75df75d643 100644 --- a/src/source/rtl_text_plugin_main_thread.test.ts +++ b/src/source/rtl_text_plugin_main_thread.test.ts @@ -3,12 +3,13 @@ import {rtlMainThreadPluginFactory} from './rtl_text_plugin_main_thread'; import {sleep} from '../util/test/util'; import {browser} from '../util/browser'; import {Dispatcher} from '../util/dispatcher'; - +import {RTLPluginStatus, PluginState, SyncRTLPluginStateMessageName} from './rtl_text_plugin_status'; const rtlMainThreadPlugin = rtlMainThreadPluginFactory(); describe('RTLMainThreadPlugin', () => { let server: FakeServer; let broadcastSpy: jest.SpyInstance; + const url = 'http://example.com/plugin'; beforeEach(() => { server = fakeServer.create(); global.fetch = null; @@ -17,6 +18,32 @@ describe('RTLMainThreadPlugin', () => { broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(() => { return Promise.resolve({} as any); }); }); + function broadcastMockSuccess(message: string, payload: PluginState): Promise { + if (message === SyncRTLPluginStateMessageName) { + + if (payload.pluginStatus === 'loading') { + const resultState: PluginState = { + pluginStatus: 'loaded', + pluginURL: payload.pluginURL + }; + return Promise.resolve([resultState]); + } + } + } + + function broadcastMockFailure(message: string, payload: PluginState): Promise { + if (message === SyncRTLPluginStateMessageName) { + + if (payload.pluginStatus === 'loading') { + const resultState: PluginState = { + pluginStatus: 'error', + pluginURL: payload.pluginURL + }; + return Promise.resolve([resultState]); + } + } + } + afterEach(() => { server.restore(); broadcastSpy.mockRestore(); @@ -28,56 +55,48 @@ describe('RTLMainThreadPlugin', () => { }); it('should set the RTL text plugin and download it', async () => { - const url = 'http://example.com/plugin'; - server.respondWith(new ArrayBuffer(0)); - - const promise = rtlMainThreadPlugin.setRTLTextPlugin(url); - await sleep(0); - server.respond(); - await promise; + broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockSuccess as any); + await rtlMainThreadPlugin.setRTLTextPlugin(url); expect(rtlMainThreadPlugin.url).toEqual(url); }); it('should set the RTL text plugin but deffer downloading', async () => { - const url = 'http://example.com/plugin'; await rtlMainThreadPlugin.setRTLTextPlugin(url, true); expect(server.requests).toHaveLength(0); expect(rtlMainThreadPlugin.status).toBe('deferred'); + expect(broadcastSpy).toHaveBeenCalledWith(SyncRTLPluginStateMessageName, {pluginStatus: 'deferred', pluginURL: url}); }); it('should throw if the plugin is already set', async () => { - const url = 'http://example.com/plugin'; await rtlMainThreadPlugin.setRTLTextPlugin(url, true); await expect(rtlMainThreadPlugin.setRTLTextPlugin(url)).rejects.toThrow('setRTLTextPlugin cannot be called multiple times.'); }); it('should throw if the plugin url is not set', async () => { const spy = jest.spyOn(browser, 'resolveURL').mockImplementation(() => { return ''; }); - await expect(rtlMainThreadPlugin.setRTLTextPlugin(null)).rejects.toThrow('rtl-text-plugin cannot be downloaded unless a pluginURL is specified'); + await expect(rtlMainThreadPlugin.setRTLTextPlugin(null)).rejects.toThrow('requested url null is invalid'); spy.mockRestore(); }); it('should be in error state if download fails', async () => { - const url = 'http://example.com/plugin'; - server.respondWith(request => request.respond(404)); + broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockFailure as any); const promise = rtlMainThreadPlugin.setRTLTextPlugin(url); - await sleep(0); - server.respond(); - await promise; + await expect(promise).rejects.toThrow(`worker failed to load ${url}`); expect(rtlMainThreadPlugin.url).toEqual(url); expect(rtlMainThreadPlugin.status).toBe('error'); }); it('should lazy load the plugin if deferred', async () => { - const url = 'http://example.com/plugin'; - server.respondWith(new ArrayBuffer(0)); + // use success spy to make sure test case does not throw exception + broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockSuccess as any); await rtlMainThreadPlugin.setRTLTextPlugin(url, true); - expect(server.requests).toHaveLength(0); expect(rtlMainThreadPlugin.status).toBe('deferred'); - const promise = rtlMainThreadPlugin.lazyLoad(); - await sleep(0); - server.respond(); - await promise; - expect(rtlMainThreadPlugin.status).toBe('loaded'); + + // this is really a fire and forget + rtlMainThreadPlugin.lazyLoad(); + window.setTimeout(() => { + expect(broadcastSpy).toHaveBeenCalledWith(SyncRTLPluginStateMessageName, {pluginStatus: 'loading', pluginURL: url}); + expect(rtlMainThreadPlugin.status).toBe('loaded'); + }, 10); }); }); diff --git a/src/source/rtl_text_plugin_main_thread.ts b/src/source/rtl_text_plugin_main_thread.ts index 44198c314c..c1461e00ca 100644 --- a/src/source/rtl_text_plugin_main_thread.ts +++ b/src/source/rtl_text_plugin_main_thread.ts @@ -32,6 +32,9 @@ class RTLMainThreadPlugin extends Evented { } this.url = browser.resolveURL(url); + if (!this.url) { + throw new Error(`requested url ${url} is invalid`); + } if (this.status === 'unavailable') { // from initial state: @@ -58,7 +61,7 @@ class RTLMainThreadPlugin extends Evented { async _download() : Promise { const workerResults = await this._syncState('loading'); if (workerResults.length > 0) { - const workerResult = workerResults[0]; + const workerResult: PluginState = workerResults[0]; this.status = workerResult.pluginStatus; // expect worker to return 'loaded' @@ -69,7 +72,7 @@ class RTLMainThreadPlugin extends Evented { if (workerResult.error) { throw workerResult.error; } else { - throw new Error(`worker failed '${SyncRTLPluginStateMessageName}' for unknown reason`); + throw new Error(`worker failed to load ${this.url}`); } } } else { From 04e5e2fd72d87cbf6231bc3a2798c594bae4ba2a Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Fri, 1 Mar 2024 13:04:32 -0800 Subject: [PATCH 05/30] test cases --- .../rtl_text_plugin_main_thread.test.ts | 26 +++++++++++++++---- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/src/source/rtl_text_plugin_main_thread.test.ts b/src/source/rtl_text_plugin_main_thread.test.ts index 75df75d643..0fb66290c5 100644 --- a/src/source/rtl_text_plugin_main_thread.test.ts +++ b/src/source/rtl_text_plugin_main_thread.test.ts @@ -58,11 +58,11 @@ describe('RTLMainThreadPlugin', () => { broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockSuccess as any); await rtlMainThreadPlugin.setRTLTextPlugin(url); expect(rtlMainThreadPlugin.url).toEqual(url); + expect(rtlMainThreadPlugin.status).toBe('loaded'); }); it('should set the RTL text plugin but deffer downloading', async () => { await rtlMainThreadPlugin.setRTLTextPlugin(url, true); - expect(server.requests).toHaveLength(0); expect(rtlMainThreadPlugin.status).toBe('deferred'); expect(broadcastSpy).toHaveBeenCalledWith(SyncRTLPluginStateMessageName, {pluginStatus: 'deferred', pluginURL: url}); }); @@ -94,9 +94,25 @@ describe('RTLMainThreadPlugin', () => { // this is really a fire and forget rtlMainThreadPlugin.lazyLoad(); - window.setTimeout(() => { - expect(broadcastSpy).toHaveBeenCalledWith(SyncRTLPluginStateMessageName, {pluginStatus: 'loading', pluginURL: url}); - expect(rtlMainThreadPlugin.status).toBe('loaded'); - }, 10); + await sleep(1); + expect(broadcastSpy).toHaveBeenCalledWith(SyncRTLPluginStateMessageName, {pluginStatus: 'loading', pluginURL: url}); + expect(rtlMainThreadPlugin.status).toBe('loaded'); + }); + + it('should set status to requested if RTL plugin was not set', async () => { + rtlMainThreadPlugin.lazyLoad(); + expect(rtlMainThreadPlugin.status).toBe('requested'); + }); + + it('should immediately download if RTL plugin was already requested, ignoring deferred:true', async () => { + broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockSuccess as any); + rtlMainThreadPlugin.lazyLoad(); + expect(rtlMainThreadPlugin.status).toBe('requested'); + await sleep(1); + + // notice even when deferred is true, it should download because already requested + await rtlMainThreadPlugin.setRTLTextPlugin(url, true); + expect(rtlMainThreadPlugin.status).toBe('loaded'); + expect(broadcastSpy).toHaveBeenCalledWith(SyncRTLPluginStateMessageName, {pluginStatus: 'loading', pluginURL: url}); }); }); From 7719c781792bdd008f0420dffae92cec80600584 Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Fri, 1 Mar 2024 13:13:11 -0800 Subject: [PATCH 06/30] change log --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f131eeac98..1b584fba46 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ - Set text color to ensure contrast in the attribution pill ([3737](https://github.com/maplibre/maplibre-gl-js/pull/3737)) - Fix memory leak in Worker when map is removed ([3734](https://github.com/maplibre/maplibre-gl-js/pull/3734)) - Fix issue with `FullscreenControl` when MapLibre is within a [ShadowRoot](https://developer.mozilla.org/en-US/docs/Web/API/ShadowRoot) ([#3573](https://github.com/maplibre/maplibre-gl-js/pull/3573)) +- Fix performance regression with `setRTLTextPlugin` which can cause 1 or 2 extra frames to render. ([#3728](https://github.com/maplibre/maplibre-gl-js/pull/3728)) ## 4.0.2 From 34696ae9c2655a63270363fa50587177658b9898 Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Fri, 1 Mar 2024 14:48:53 -0800 Subject: [PATCH 07/30] worker tests --- src/source/worker.test.ts | 129 ++++++++++++++++++++++++++++---------- src/source/worker.ts | 14 ++--- 2 files changed, 103 insertions(+), 40 deletions(-) diff --git a/src/source/worker.test.ts b/src/source/worker.test.ts index 38c39d273e..de2f07254a 100644 --- a/src/source/worker.test.ts +++ b/src/source/worker.test.ts @@ -6,6 +6,7 @@ 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, SyncRTLPluginStateMessageName} from './rtl_text_plugin_status'; class WorkerSourceMock implements WorkerSource { availableImages: string[]; @@ -28,6 +29,101 @@ describe('Worker register RTLTextPlugin', () => { let worker: Worker; let _self: WorkerGlobalScopeInterface & ActorTarget; + beforeEach(() => { + _self = { + addEventListener() {}, + importScripts() {} + } as any; + worker = new Worker(_self); + global.fetch = null; + }); + + test('should not throw and set values in plugin', () => { + jest.spyOn(rtlWorkerPlugin, 'isParsed').mockImplementation(() => { + return false; + }); + + 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', () => { + rtlWorkerPlugin.setState({ + pluginURL: '', + pluginStatus: 'unavailable' + } + ); + const mockMessage: PluginState = { + pluginURL: '', + pluginStatus: 'deferred' + }; + // force calling to private method + (worker as any)['_syncRTLPluginState']('', mockMessage); + expect(rtlWorkerPlugin.getRTLTextPluginStatus()).toBe('deferred'); + }); + + test('should download RTL plugin when "loading" message is received', async () => { + rtlWorkerPlugin.setState({ + pluginURL: '', + pluginStatus: 'deferred' + }); + + const mockURL = 'https://somehost/somescript'; + const mockMessage: PluginState = { + pluginURL: mockURL, + pluginStatus: 'loading' + }; + + // isParse() to return false at first + jest.spyOn(rtlWorkerPlugin, 'isParsed').mockImplementation(() => { + return false; + }); + + const importSpy = jest.spyOn(worker.self, 'importScripts').mockImplementation(() => { + // after importing isParse() to return true + jest.spyOn(rtlWorkerPlugin, 'isParsed').mockImplementation(() => { + return true; + }); + }); + + // force calling to private method + const syncResult: PluginState = await (worker as any)['_syncRTLPluginState']('', mockMessage); + expect(rtlWorkerPlugin.getRTLTextPluginStatus()).toBe('loaded'); + expect(importSpy).toHaveBeenCalledWith(mockURL); + expect(syncResult.error).toBeUndefined(); + expect(syncResult.pluginURL).toBe(mockURL); + expect(syncResult.pluginStatus).toBe('loaded'); + }); +}); + +describe('Worker generic testing', () => { + let worker: Worker; + let _self: WorkerGlobalScopeInterface & ActorTarget; + beforeEach(() => { _self = { addEventListener() {} @@ -85,39 +181,6 @@ describe('Worker register RTLTextPlugin', () => { worker.actor.messageHandlers['loadTile']('999', {type: extenalSourceName} as WorkerTileParameters); }); - test('should not throw and set values in plugin', () => { - jest.spyOn(rtlWorkerPlugin, 'isParsed').mockImplementation(() => { - return false; - }); - - 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('Referrer is set', () => { worker.actor.messageHandlers['setReferrer']('fakeId', 'myMap'); expect(worker.referrer).toBe('myMap'); diff --git a/src/source/worker.ts b/src/source/worker.ts index 125daf50f2..4e82dfc19d 100644 --- a/src/source/worker.ts +++ b/src/source/worker.ts @@ -6,7 +6,7 @@ import {rtlWorkerPlugin, RTLTextPlugin} from './rtl_text_plugin_worker'; import {GeoJSONWorkerSource, LoadGeoJSONParameters} from './geojson_worker_source'; import {isWorker} from '../util/util'; import {addProtocol, removeProtocol} from './protocol_crud'; -import {RTLPluginStatus, SyncRTLPluginStateMessageName} from './rtl_text_plugin_status'; +import {PluginState, SyncRTLPluginStateMessageName} from './rtl_text_plugin_status'; import type { WorkerSource, WorkerSourceConstructor, @@ -17,7 +17,6 @@ import type { import type {WorkerGlobalScopeInterface} from '../util/web_worker'; import type {LayerSpecification} from '@maplibre/maplibre-gl-style-spec'; -import type {PluginState} from './rtl_text_plugin_status'; import type {ClusterIDAndSource, GetClusterLeavesParams, RemoveSourceParams, UpdateLayersParamaeters} from '../util/actor_messages'; /** @@ -182,18 +181,19 @@ export default class Worker { } private async _syncRTLPluginState(mapId: string, incomingState: PluginState): Promise { - const resultState: PluginState = { - pluginStatus: rtlWorkerPlugin.pluginStatus, - pluginURL: rtlWorkerPlugin.pluginURL - }; + const resultState = incomingState; if (incomingState.pluginStatus === 'loading' && !rtlWorkerPlugin.isParsed() && incomingState.pluginURL != null) { this.self.importScripts(incomingState.pluginURL); - if (rtlWorkerPlugin.isParsed()) { + const complete = rtlWorkerPlugin.isParsed(); + if (complete) { resultState.pluginStatus = 'loaded'; resultState.pluginURL = incomingState.pluginURL; rtlWorkerPlugin.setState(resultState); } + } else { + // simply sync and done + rtlWorkerPlugin.setState(resultState); } return resultState; From 30a512eaf94af6d896d5e216f6b3e79bec94663f Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Fri, 1 Mar 2024 16:13:24 -0800 Subject: [PATCH 08/30] add change in style.ts --- src/style/style.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/style/style.ts b/src/style/style.ts index 9326547779..30166da127 100644 --- a/src/style/style.ts +++ b/src/style/style.ts @@ -20,6 +20,7 @@ import {GeoJSONSource} from '../source/geojson_source'; import {latest as styleSpec, derefLayers as deref, emptyStyle, diff as diffStyles, DiffCommand} from '@maplibre/maplibre-gl-style-spec'; import {getGlobalWorkerPool} from '../util/global_worker_pool'; import {rtlMainThreadPluginFactory} from '../source/rtl_text_plugin_main_thread'; +import {RTLPluginLoadedEventName} from '../source/rtl_text_plugin_status'; import {PauseablePlacement} from './pauseable_placement'; import {ZoomHistory} from './zoom_history'; import {CrossTileSymbolIndex} from '../symbol/cross_tile_symbol_index'; @@ -234,7 +235,7 @@ export class Style extends Evented { this._resetUpdates(); this.dispatcher.broadcast('setReferrer', getReferrer()); - rtlMainThreadPluginFactory().on('pluginStateChange', this._rtlTextPluginStateChange); + rtlMainThreadPluginFactory().on(RTLPluginLoadedEventName, this._rtlPluginLoaded); this.on('data', (event) => { if (event.dataType !== 'source' || event.sourceDataType !== 'metadata') { @@ -260,7 +261,7 @@ export class Style extends Evented { }); } - _rtlTextPluginStateChange = () => { + _rtlPluginLoaded = () => { for (const id in this.sourceCaches) { const sourceType = this.sourceCaches[id].getSource().type; if (sourceType === 'vector' || sourceType === 'geojson') { @@ -1481,7 +1482,7 @@ export class Style extends Evented { this._spriteRequest.abort(); this._spriteRequest = null; } - rtlMainThreadPluginFactory().off('pluginStateChange', this._rtlTextPluginStateChange); + rtlMainThreadPluginFactory().off(RTLPluginLoadedEventName, this._rtlPluginLoaded); for (const layerId in this._layers) { const layer: StyleLayer = this._layers[layerId]; layer.setEventedParent(null); From f7108afcc7354931bf86a40662e43db2440d6067 Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Fri, 1 Mar 2024 16:17:13 -0800 Subject: [PATCH 09/30] update style test --- src/style/style.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/style/style.test.ts b/src/style/style.test.ts index 3f4449376a..d6aaf47358 100644 --- a/src/style/style.test.ts +++ b/src/style/style.test.ts @@ -15,6 +15,7 @@ import {EvaluationParameters} from './evaluation_parameters'; import {LayerSpecification, GeoJSONSourceSpecification, FilterSpecification, SourceSpecification, StyleSpecification, SymbolLayerSpecification, TerrainSpecification} from '@maplibre/maplibre-gl-style-spec'; import {GeoJSONSource} from '../source/geojson_source'; import {sleep} from '../util/test/util'; +import {RTLPluginLoadedEventName} from '../source/rtl_text_plugin_status'; function createStyleJSON(properties?): StyleSpecification { return extend({ @@ -118,7 +119,7 @@ describe('Style', () => { jest.spyOn(style.sourceCaches['raster'], 'reload'); jest.spyOn(style.sourceCaches['vector'], 'reload'); - rtlMainThreadPluginFactory().fire(new Event('pluginStateChange')); + rtlMainThreadPluginFactory().fire(new Event(RTLPluginLoadedEventName)); expect(style.sourceCaches['raster'].reload).not.toHaveBeenCalled(); expect(style.sourceCaches['vector'].reload).toHaveBeenCalled(); From 646369f1bf78ebc3c03ddb54932c98b06922acc8 Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Fri, 1 Mar 2024 16:42:42 -0800 Subject: [PATCH 10/30] update UT --- src/source/worker.test.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/source/worker.test.ts b/src/source/worker.test.ts index de2f07254a..1f90bb7cd5 100644 --- a/src/source/worker.test.ts +++ b/src/source/worker.test.ts @@ -81,8 +81,8 @@ describe('Worker register RTLTextPlugin', () => { pluginURL: '', pluginStatus: 'deferred' }; - // force calling to private method - (worker as any)['_syncRTLPluginState']('', mockMessage); + + worker.actor.messageHandlers[SyncRTLPluginStateMessageName]('', mockMessage); expect(rtlWorkerPlugin.getRTLTextPluginStatus()).toBe('deferred'); }); @@ -110,8 +110,7 @@ describe('Worker register RTLTextPlugin', () => { }); }); - // force calling to private method - const syncResult: PluginState = await (worker as any)['_syncRTLPluginState']('', mockMessage); + const syncResult: PluginState = await worker.actor.messageHandlers[SyncRTLPluginStateMessageName]('', mockMessage) as any; expect(rtlWorkerPlugin.getRTLTextPluginStatus()).toBe('loaded'); expect(importSpy).toHaveBeenCalledWith(mockURL); expect(syncResult.error).toBeUndefined(); From 221f67ea6ee74036f4f66b609fa5ee54454e62ff Mon Sep 17 00:00:00 2001 From: Harel M Date: Sat, 2 Mar 2024 18:16:26 +0200 Subject: [PATCH 11/30] Update src/source/rtl_text_plugin_main_thread.test.ts --- src/source/rtl_text_plugin_main_thread.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/source/rtl_text_plugin_main_thread.test.ts b/src/source/rtl_text_plugin_main_thread.test.ts index 0fb66290c5..9fa808f7c3 100644 --- a/src/source/rtl_text_plugin_main_thread.test.ts +++ b/src/source/rtl_text_plugin_main_thread.test.ts @@ -3,7 +3,7 @@ import {rtlMainThreadPluginFactory} from './rtl_text_plugin_main_thread'; import {sleep} from '../util/test/util'; import {browser} from '../util/browser'; import {Dispatcher} from '../util/dispatcher'; -import {RTLPluginStatus, PluginState, SyncRTLPluginStateMessageName} from './rtl_text_plugin_status'; +import {PluginState, SyncRTLPluginStateMessageName} from './rtl_text_plugin_status'; const rtlMainThreadPlugin = rtlMainThreadPluginFactory(); describe('RTLMainThreadPlugin', () => { From c9e3ba681529c0422e3181061dd39bc9754cd6f0 Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Mon, 4 Mar 2024 09:48:54 -0800 Subject: [PATCH 12/30] PR comments --- .../rtl_text_plugin_main_thread.test.ts | 4 +- src/source/rtl_text_plugin_main_thread.ts | 21 +++----- src/source/rtl_text_plugin_status.ts | 3 -- src/source/worker.test.ts | 48 ++++++++++++++----- src/source/worker.ts | 28 +++++++---- 5 files changed, 62 insertions(+), 42 deletions(-) diff --git a/src/source/rtl_text_plugin_main_thread.test.ts b/src/source/rtl_text_plugin_main_thread.test.ts index 0fb66290c5..d5e2253b91 100644 --- a/src/source/rtl_text_plugin_main_thread.test.ts +++ b/src/source/rtl_text_plugin_main_thread.test.ts @@ -18,7 +18,7 @@ describe('RTLMainThreadPlugin', () => { broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(() => { return Promise.resolve({} as any); }); }); - function broadcastMockSuccess(message: string, payload: PluginState): Promise { + function broadcastMockSuccess(message: typeof SyncRTLPluginStateMessageName, payload: PluginState): Promise { if (message === SyncRTLPluginStateMessageName) { if (payload.pluginStatus === 'loading') { @@ -31,7 +31,7 @@ describe('RTLMainThreadPlugin', () => { } } - function broadcastMockFailure(message: string, payload: PluginState): Promise { + function broadcastMockFailure(message: typeof SyncRTLPluginStateMessageName, payload: PluginState): Promise { if (message === SyncRTLPluginStateMessageName) { if (payload.pluginStatus === 'loading') { diff --git a/src/source/rtl_text_plugin_main_thread.ts b/src/source/rtl_text_plugin_main_thread.ts index c1461e00ca..4ab6a02f8b 100644 --- a/src/source/rtl_text_plugin_main_thread.ts +++ b/src/source/rtl_text_plugin_main_thread.ts @@ -48,17 +48,17 @@ class RTLMainThreadPlugin extends Evented { } else { // immediate download - return this._download(); + return this._requestImport(); } } else if (this.status === 'requested') { // already requested, start downloading - return this._download(); + return this._requestImport(); } } - /** Download RTL plugin by sending a message to worker and process its response */ - async _download() : Promise { + /** Send a message to worker which will import the RTL plugin script */ + async _requestImport() : Promise { const workerResults = await this._syncState('loading'); if (workerResults.length > 0) { const workerResult: PluginState = workerResults[0]; @@ -68,17 +68,8 @@ class RTLMainThreadPlugin extends Evented { if (workerResult.pluginStatus === 'loaded') { this.fire(new Event(RTLPluginLoadedEventName)); } else { - // failed scenario: returned, but bad status - if (workerResult.error) { - throw workerResult.error; - } else { - throw new Error(`worker failed to load ${this.url}`); - } + throw new Error(`worker failed to load ${this.url}, worker status=${workerResult.pluginStatus}`); } - } else { - // failed scenario(edge case): worker did not respond - this.status = 'error'; - throw new Error(`worker did not respond to message: ${SyncRTLPluginStateMessageName}`); } } @@ -87,7 +78,7 @@ class RTLMainThreadPlugin extends Evented { if (this.status === 'unavailable') { this.status = 'requested'; } else if (this.status === 'deferred') { - this._download(); + this._requestImport(); } } } diff --git a/src/source/rtl_text_plugin_status.ts b/src/source/rtl_text_plugin_status.ts index 04a69858f3..52d9a09aa8 100644 --- a/src/source/rtl_text_plugin_status.ts +++ b/src/source/rtl_text_plugin_status.ts @@ -27,9 +27,6 @@ export type RTLPluginStatus = export type PluginState = { pluginStatus: RTLPluginStatus; pluginURL: string; - - /** Optional error object that carries error from worker to main thread */ - error?: Error; }; export const RTLPluginLoadedEventName = 'RTLPluginLoaded'; diff --git a/src/source/worker.test.ts b/src/source/worker.test.ts index 1f90bb7cd5..d5932962b1 100644 --- a/src/source/worker.test.ts +++ b/src/source/worker.test.ts @@ -25,7 +25,7 @@ class WorkerSourceMock implements WorkerSource { } } -describe('Worker register RTLTextPlugin', () => { +describe('Worker RTLTextPlugin', () => { let worker: Worker; let _self: WorkerGlobalScopeInterface & ActorTarget; @@ -36,13 +36,17 @@ describe('Worker register RTLTextPlugin', () => { } as any; worker = new Worker(_self); global.fetch = null; - }); - - test('should not throw and set values in plugin', () => { + 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', @@ -71,18 +75,18 @@ describe('Worker register RTLTextPlugin', () => { }).toThrow('RTL text plugin already registered.'); }); - test('should move RTL plugin from unavailable to deferred', () => { + test('should move RTL plugin from unavailable to deferred', async () => { rtlWorkerPlugin.setState({ pluginURL: '', pluginStatus: 'unavailable' } ); const mockMessage: PluginState = { - pluginURL: '', + pluginURL: 'https://somehost/somescript', pluginStatus: 'deferred' }; - worker.actor.messageHandlers[SyncRTLPluginStateMessageName]('', mockMessage); + await worker.actor.messageHandlers[SyncRTLPluginStateMessageName]('', mockMessage); expect(rtlWorkerPlugin.getRTLTextPluginStatus()).toBe('deferred'); }); @@ -98,11 +102,6 @@ describe('Worker register RTLTextPlugin', () => { pluginStatus: 'loading' }; - // isParse() to return false at first - jest.spyOn(rtlWorkerPlugin, 'isParsed').mockImplementation(() => { - return false; - }); - const importSpy = jest.spyOn(worker.self, 'importScripts').mockImplementation(() => { // after importing isParse() to return true jest.spyOn(rtlWorkerPlugin, 'isParsed').mockImplementation(() => { @@ -113,10 +112,33 @@ describe('Worker register RTLTextPlugin', () => { const syncResult: PluginState = await worker.actor.messageHandlers[SyncRTLPluginStateMessageName]('', mockMessage) as any; expect(rtlWorkerPlugin.getRTLTextPluginStatus()).toBe('loaded'); expect(importSpy).toHaveBeenCalledWith(mockURL); - expect(syncResult.error).toBeUndefined(); + expect(syncResult.pluginURL).toBe(mockURL); expect(syncResult.pluginStatus).toBe('loaded'); }); + + 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' + }; + + const workerResult: PluginState = await worker.actor.messageHandlers[SyncRTLPluginStateMessageName]('', mockMessage) as any; + expect(rtlWorkerPlugin.getRTLTextPluginStatus()).toBe('loaded'); + expect(rtlWorkerPlugin.getPluginURL()).toBe(originalUrl); + + expect(workerResult.pluginStatus).toBe('loaded'); + expect(workerResult.pluginURL).toBe(originalUrl); + }); }); describe('Worker generic testing', () => { diff --git a/src/source/worker.ts b/src/source/worker.ts index 4e82dfc19d..71f0976f87 100644 --- a/src/source/worker.ts +++ b/src/source/worker.ts @@ -183,17 +183,27 @@ export default class Worker { private async _syncRTLPluginState(mapId: string, incomingState: PluginState): Promise { const resultState = incomingState; - if (incomingState.pluginStatus === 'loading' && !rtlWorkerPlugin.isParsed() && incomingState.pluginURL != null) { - this.self.importScripts(incomingState.pluginURL); - const complete = rtlWorkerPlugin.isParsed(); - if (complete) { - resultState.pluginStatus = 'loaded'; - resultState.pluginURL = incomingState.pluginURL; - rtlWorkerPlugin.setState(resultState); - } - } else { + // Parsed plugin cannot be changed; + // and cannot do anything if url is blank so both cases just return its current state. + if (rtlWorkerPlugin.isParsed() || !incomingState.pluginURL) { + return { + pluginURL: rtlWorkerPlugin.getPluginURL(), + pluginStatus: rtlWorkerPlugin.getRTLTextPluginStatus() + }; + } + + if (incomingState.pluginStatus !== 'loading') { // simply sync and done rtlWorkerPlugin.setState(resultState); + return resultState; + } + + this.self.importScripts(incomingState.pluginURL); + const complete = rtlWorkerPlugin.isParsed(); + if (complete) { + resultState.pluginStatus = 'loaded'; + resultState.pluginURL = incomingState.pluginURL; + rtlWorkerPlugin.setState(resultState); } return resultState; From c586e8060452d860b0cc487196686396a152f02d Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Mon, 4 Mar 2024 13:11:40 -0800 Subject: [PATCH 13/30] PR comments --- .../rtl_text_plugin_main_thread.test.ts | 7 +++--- src/source/rtl_text_plugin_main_thread.ts | 16 ++++++++++-- src/source/rtl_text_plugin_status.ts | 2 +- src/source/worker.test.ts | 3 ++- src/source/worker.ts | 25 ++++++++++++------- src/util/actor_messages.ts | 5 +++- 6 files changed, 41 insertions(+), 17 deletions(-) diff --git a/src/source/rtl_text_plugin_main_thread.test.ts b/src/source/rtl_text_plugin_main_thread.test.ts index 5fe6412983..5fc3152e1d 100644 --- a/src/source/rtl_text_plugin_main_thread.test.ts +++ b/src/source/rtl_text_plugin_main_thread.test.ts @@ -3,7 +3,8 @@ import {rtlMainThreadPluginFactory} from './rtl_text_plugin_main_thread'; import {sleep} from '../util/test/util'; import {browser} from '../util/browser'; import {Dispatcher} from '../util/dispatcher'; -import {PluginState, SyncRTLPluginStateMessageName} from './rtl_text_plugin_status'; +import {PluginState} from './rtl_text_plugin_status'; +import {MessageType, SyncRTLPluginStateMessageName} from '../util/actor_messages'; const rtlMainThreadPlugin = rtlMainThreadPluginFactory(); describe('RTLMainThreadPlugin', () => { @@ -18,7 +19,7 @@ describe('RTLMainThreadPlugin', () => { broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(() => { return Promise.resolve({} as any); }); }); - function broadcastMockSuccess(message: typeof SyncRTLPluginStateMessageName, payload: PluginState): Promise { + function broadcastMockSuccess(message: MessageType, payload: PluginState): Promise { if (message === SyncRTLPluginStateMessageName) { if (payload.pluginStatus === 'loading') { @@ -31,7 +32,7 @@ describe('RTLMainThreadPlugin', () => { } } - function broadcastMockFailure(message: typeof SyncRTLPluginStateMessageName, payload: PluginState): Promise { + function broadcastMockFailure(message: MessageType, payload: PluginState): Promise { if (message === SyncRTLPluginStateMessageName) { if (payload.pluginStatus === 'loading') { diff --git a/src/source/rtl_text_plugin_main_thread.ts b/src/source/rtl_text_plugin_main_thread.ts index 4ab6a02f8b..ddf1e4d9ce 100644 --- a/src/source/rtl_text_plugin_main_thread.ts +++ b/src/source/rtl_text_plugin_main_thread.ts @@ -1,9 +1,13 @@ import {browser} from '../util/browser'; import {Event, Evented} from '../util/evented'; -import {RTLPluginStatus, RTLPluginLoadedEventName, SyncRTLPluginStateMessageName, PluginState} from './rtl_text_plugin_status'; +import {RTLPluginStatus, RTLPluginLoadedEventName, PluginState} from './rtl_text_plugin_status'; import {Dispatcher, getGlobalDispatcher} from '../util/dispatcher'; +import {getArrayBuffer} from '../util/ajax'; +import {SyncRTLPluginStateMessageName} from '../util/actor_messages'; +import {WorkerPool} from '../util/worker_pool'; + class RTLMainThreadPlugin extends Evented { status: RTLPluginStatus = 'unavailable'; url: string = null; @@ -52,13 +56,21 @@ class RTLMainThreadPlugin extends Evented { } } else if (this.status === 'requested') { - // already requested, start downloading return this._requestImport(); } } /** Send a message to worker which will import the RTL plugin script */ async _requestImport() : Promise { + + // Reference PR: https://github.com/mapbox/mapbox-gl-js/pull/9122 + // if have more than 1 workers, it is better to load once in main thread to warm up browser cache + // so all workers can use it --- even though the result of getArrayBuffer is not being used here. + // Otherwise, just let worker importScript once. + if (WorkerPool.workerCount > 1) { + await getArrayBuffer({url: this.url}, new AbortController()); + } + const workerResults = await this._syncState('loading'); if (workerResults.length > 0) { const workerResult: PluginState = workerResults[0]; diff --git a/src/source/rtl_text_plugin_status.ts b/src/source/rtl_text_plugin_status.ts index 52d9a09aa8..3b7a3f4630 100644 --- a/src/source/rtl_text_plugin_status.ts +++ b/src/source/rtl_text_plugin_status.ts @@ -30,4 +30,4 @@ export type PluginState = { }; export const RTLPluginLoadedEventName = 'RTLPluginLoaded'; -export const SyncRTLPluginStateMessageName = 'syncRTLPluginState'; + diff --git a/src/source/worker.test.ts b/src/source/worker.test.ts index d5932962b1..de96e61c42 100644 --- a/src/source/worker.test.ts +++ b/src/source/worker.test.ts @@ -6,7 +6,8 @@ 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, SyncRTLPluginStateMessageName} from './rtl_text_plugin_status'; +import {PluginState} from './rtl_text_plugin_status'; +import {SyncRTLPluginStateMessageName} from '../util/actor_messages'; class WorkerSourceMock implements WorkerSource { availableImages: string[]; diff --git a/src/source/worker.ts b/src/source/worker.ts index 71f0976f87..9701710004 100644 --- a/src/source/worker.ts +++ b/src/source/worker.ts @@ -6,7 +6,7 @@ import {rtlWorkerPlugin, RTLTextPlugin} from './rtl_text_plugin_worker'; import {GeoJSONWorkerSource, LoadGeoJSONParameters} from './geojson_worker_source'; import {isWorker} from '../util/util'; import {addProtocol, removeProtocol} from './protocol_crud'; -import {PluginState, SyncRTLPluginStateMessageName} from './rtl_text_plugin_status'; +import {PluginState} from './rtl_text_plugin_status'; import type { WorkerSource, WorkerSourceConstructor, @@ -17,7 +17,13 @@ import type { import type {WorkerGlobalScopeInterface} from '../util/web_worker'; import type {LayerSpecification} from '@maplibre/maplibre-gl-style-spec'; -import type {ClusterIDAndSource, GetClusterLeavesParams, RemoveSourceParams, UpdateLayersParamaeters} from '../util/actor_messages'; +import { + SyncRTLPluginStateMessageName, + type ClusterIDAndSource, + type GetClusterLeavesParams, + type RemoveSourceParams, + type UpdateLayersParamaeters +} from '../util/actor_messages'; /** * The Worker class responsidble for background thread related execution @@ -183,9 +189,8 @@ export default class Worker { private async _syncRTLPluginState(mapId: string, incomingState: PluginState): Promise { const resultState = incomingState; - // Parsed plugin cannot be changed; - // and cannot do anything if url is blank so both cases just return its current state. - if (rtlWorkerPlugin.isParsed() || !incomingState.pluginURL) { + // Parsed plugin cannot be changed, so just return its current state. + if (rtlWorkerPlugin.isParsed()) { return { pluginURL: rtlWorkerPlugin.getPluginURL(), pluginStatus: rtlWorkerPlugin.getRTLTextPluginStatus() @@ -197,16 +202,18 @@ export default class Worker { rtlWorkerPlugin.setState(resultState); return resultState; } - - this.self.importScripts(incomingState.pluginURL); + const urlToLoad = incomingState.pluginURL; + this.self.importScripts(urlToLoad); const complete = rtlWorkerPlugin.isParsed(); if (complete) { resultState.pluginStatus = 'loaded'; - resultState.pluginURL = incomingState.pluginURL; + resultState.pluginURL = urlToLoad; rtlWorkerPlugin.setState(resultState); + return resultState; } - return resultState; + // error case + throw new Error(`RTL Text Plugin failed to import scripts from ${urlToLoad}`); } private _getAvailableImages(mapId: string) { diff --git a/src/util/actor_messages.ts b/src/util/actor_messages.ts index 7de7df8e86..d8530bc9cc 100644 --- a/src/util/actor_messages.ts +++ b/src/util/actor_messages.ts @@ -3,7 +3,7 @@ import type {TileParameters, WorkerDEMTileParameters, WorkerTileParameters, Work import type {DEMData} from '../data/dem_data'; import type {StyleImage} from '../style/style_image'; import type {StyleGlyph} from '../style/style_glyph'; -import type {PluginState, SyncRTLPluginStateMessageName} from '../source/rtl_text_plugin_status'; +import type {PluginState} from '../source/rtl_text_plugin_status'; import type {LayerSpecification} from '@maplibre/maplibre-gl-style-spec'; import type {OverscaledTileID} from '../source/tile_id'; import type {GetResourceResponse, RequestParameters} from './ajax'; @@ -80,6 +80,9 @@ export type GetGlyphsResponse = { */ export type GetImagesResponse = {[_: string]: StyleImage} +/** the message name to sync RTL Plugin state. Shared by main thread and worker */ +export const SyncRTLPluginStateMessageName = 'syncRTLPluginState'; + /** * This is basically a mapping between all the calls that are made to and from the workers. * The key is the event name, the first parameter is the event input type, and the last parameter is the output type. From 8cd6a8bd3d1c88b035c0ac709107597fe0d1adaa Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Mon, 4 Mar 2024 13:46:40 -0800 Subject: [PATCH 14/30] dup lazyLoad() calls --- .../rtl_text_plugin_main_thread.test.ts | 24 ++++++++++++-- src/source/rtl_text_plugin_main_thread.ts | 31 +++++++++++++------ 2 files changed, 43 insertions(+), 12 deletions(-) diff --git a/src/source/rtl_text_plugin_main_thread.test.ts b/src/source/rtl_text_plugin_main_thread.test.ts index 5fc3152e1d..345a9531f9 100644 --- a/src/source/rtl_text_plugin_main_thread.test.ts +++ b/src/source/rtl_text_plugin_main_thread.test.ts @@ -21,7 +21,6 @@ describe('RTLMainThreadPlugin', () => { function broadcastMockSuccess(message: MessageType, payload: PluginState): Promise { if (message === SyncRTLPluginStateMessageName) { - if (payload.pluginStatus === 'loading') { const resultState: PluginState = { pluginStatus: 'loaded', @@ -34,7 +33,6 @@ describe('RTLMainThreadPlugin', () => { function broadcastMockFailure(message: MessageType, payload: PluginState): Promise { if (message === SyncRTLPluginStateMessageName) { - if (payload.pluginStatus === 'loading') { const resultState: PluginState = { pluginStatus: 'error', @@ -91,13 +89,28 @@ describe('RTLMainThreadPlugin', () => { // use success spy to make sure test case does not throw exception broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockSuccess as any); await rtlMainThreadPlugin.setRTLTextPlugin(url, true); + expect(broadcastSpy).toHaveBeenCalledWith(SyncRTLPluginStateMessageName, {pluginStatus: 'deferred', pluginURL: url}); expect(rtlMainThreadPlugin.status).toBe('deferred'); // this is really a fire and forget + // two calls to lazyLoad rtlMainThreadPlugin.lazyLoad(); await sleep(1); expect(broadcastSpy).toHaveBeenCalledWith(SyncRTLPluginStateMessageName, {pluginStatus: 'loading', pluginURL: url}); + + // two times, first for "deferred", second for 'loading' + expect(broadcastSpy).toHaveBeenCalledTimes(2); + + // second call to lazyLoad should not change anything + rtlMainThreadPlugin.lazyLoad(); + expect(broadcastSpy).toHaveBeenCalledTimes(2); + + expect(rtlMainThreadPlugin.status).toBe('loaded'); + + // 3rd call to lazyLoad should not change anything + rtlMainThreadPlugin.lazyLoad(); expect(rtlMainThreadPlugin.status).toBe('loaded'); + expect(broadcastSpy).toHaveBeenCalledTimes(2); }); it('should set status to requested if RTL plugin was not set', async () => { @@ -116,4 +129,11 @@ describe('RTLMainThreadPlugin', () => { expect(rtlMainThreadPlugin.status).toBe('loaded'); expect(broadcastSpy).toHaveBeenCalledWith(SyncRTLPluginStateMessageName, {pluginStatus: 'loading', pluginURL: url}); }); + + it('should allow multiple calls to lazyLoad', async () => { + rtlMainThreadPlugin.lazyLoad(); + expect(rtlMainThreadPlugin.status).toBe('requested'); + rtlMainThreadPlugin.lazyLoad(); + expect(rtlMainThreadPlugin.status).toBe('requested'); + }); }); diff --git a/src/source/rtl_text_plugin_main_thread.ts b/src/source/rtl_text_plugin_main_thread.ts index ddf1e4d9ce..2ed6e953fd 100644 --- a/src/source/rtl_text_plugin_main_thread.ts +++ b/src/source/rtl_text_plugin_main_thread.ts @@ -71,17 +71,27 @@ class RTLMainThreadPlugin extends Evented { await getArrayBuffer({url: this.url}, new AbortController()); } - const workerResults = await this._syncState('loading'); - if (workerResults.length > 0) { - const workerResult: PluginState = workerResults[0]; - this.status = workerResult.pluginStatus; - - // expect worker to return 'loaded' - if (workerResult.pluginStatus === 'loaded') { - this.fire(new Event(RTLPluginLoadedEventName)); - } else { - throw new Error(`worker failed to load ${this.url}, worker status=${workerResult.pluginStatus}`); + try { + const workerResults = await this._syncState('loading'); + if (workerResults.length > 0) { + + // expect all of them to be 'loaded' + const expectedStatus = 'loaded'; + const failedToLoadWorkers = workerResults.filter((workerResult) => { + return workerResult.pluginStatus !== expectedStatus; + }); + + if (failedToLoadWorkers.length > 0) { + throw new Error(failedToLoadWorkers[1].pluginStatus); + } else { + // all success + this.status = expectedStatus; + this.fire(new Event(RTLPluginLoadedEventName)); + } } + } catch (e) { + this.status = 'error'; + throw new Error(`worker failed to load ${this.url}, ${e.toString()}`); } } @@ -90,6 +100,7 @@ class RTLMainThreadPlugin extends Evented { if (this.status === 'unavailable') { this.status = 'requested'; } else if (this.status === 'deferred') { + console.log(`lazy load, status = ${this.status}, calling _requestImport`); this._requestImport(); } } From 025ef4d2546083f0a0ee9e74170ce06f990ac60a Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Mon, 4 Mar 2024 14:01:16 -0800 Subject: [PATCH 15/30] multiple --- .../rtl_text_plugin_main_thread.test.ts | 23 +++++++++++++++++++ src/source/rtl_text_plugin_main_thread.ts | 6 ++--- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/src/source/rtl_text_plugin_main_thread.test.ts b/src/source/rtl_text_plugin_main_thread.test.ts index 345a9531f9..b355a7e48b 100644 --- a/src/source/rtl_text_plugin_main_thread.test.ts +++ b/src/source/rtl_text_plugin_main_thread.test.ts @@ -43,6 +43,23 @@ describe('RTLMainThreadPlugin', () => { } } + /** return two results, one success one failure */ + function broadcastMockMix(message: MessageType, payload: PluginState): Promise { + if (message === SyncRTLPluginStateMessageName) { + if (payload.pluginStatus === 'loading') { + const resultState0: PluginState = { + pluginStatus: 'loaded', + pluginURL: payload.pluginURL + }; + const resultState1: PluginState = { + pluginStatus: 'error', + pluginURL: payload.pluginURL + }; + return Promise.resolve([resultState0, resultState1]); + } + } + } + afterEach(() => { server.restore(); broadcastSpy.mockRestore(); @@ -136,4 +153,10 @@ describe('RTLMainThreadPlugin', () => { rtlMainThreadPlugin.lazyLoad(); expect(rtlMainThreadPlugin.status).toBe('requested'); }); + + it('should throw error for mixed success and failure', async () => { + broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockMix as any); + const promise = rtlMainThreadPlugin.setRTLTextPlugin(url); + await expect(promise).rejects.toThrow(`worker failed to load ${url}, worker status is error`); + }); }); diff --git a/src/source/rtl_text_plugin_main_thread.ts b/src/source/rtl_text_plugin_main_thread.ts index 2ed6e953fd..831943489b 100644 --- a/src/source/rtl_text_plugin_main_thread.ts +++ b/src/source/rtl_text_plugin_main_thread.ts @@ -51,7 +51,6 @@ class RTLMainThreadPlugin extends Evented { this._syncState(this.status); } else { - // immediate download return this._requestImport(); } @@ -82,7 +81,7 @@ class RTLMainThreadPlugin extends Evented { }); if (failedToLoadWorkers.length > 0) { - throw new Error(failedToLoadWorkers[1].pluginStatus); + throw failedToLoadWorkers[0].pluginStatus; } else { // all success this.status = expectedStatus; @@ -91,7 +90,7 @@ class RTLMainThreadPlugin extends Evented { } } catch (e) { this.status = 'error'; - throw new Error(`worker failed to load ${this.url}, ${e.toString()}`); + throw new Error(`worker failed to load ${this.url}, worker status is ${e.toString()}`); } } @@ -100,7 +99,6 @@ class RTLMainThreadPlugin extends Evented { if (this.status === 'unavailable') { this.status = 'requested'; } else if (this.status === 'deferred') { - console.log(`lazy load, status = ${this.status}, calling _requestImport`); this._requestImport(); } } From 89c12b9eb4ba59906a5c88425e7e71b610ed32a7 Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Wed, 6 Mar 2024 10:38:24 -0800 Subject: [PATCH 16/30] PR --- .../rtl_text_plugin_main_thread.test.ts | 3 +- src/source/rtl_text_plugin_main_thread.ts | 31 +++++++++---------- src/source/rtl_text_plugin_worker.ts | 7 +++++ src/source/worker.test.ts | 7 ++--- src/source/worker.ts | 23 ++++++-------- src/util/actor_messages.ts | 5 +-- 6 files changed, 37 insertions(+), 39 deletions(-) diff --git a/src/source/rtl_text_plugin_main_thread.test.ts b/src/source/rtl_text_plugin_main_thread.test.ts index b355a7e48b..dee69eaf63 100644 --- a/src/source/rtl_text_plugin_main_thread.test.ts +++ b/src/source/rtl_text_plugin_main_thread.test.ts @@ -4,13 +4,14 @@ import {sleep} from '../util/test/util'; import {browser} from '../util/browser'; import {Dispatcher} from '../util/dispatcher'; import {PluginState} from './rtl_text_plugin_status'; -import {MessageType, SyncRTLPluginStateMessageName} from '../util/actor_messages'; +import {MessageType} from '../util/actor_messages'; const rtlMainThreadPlugin = rtlMainThreadPluginFactory(); describe('RTLMainThreadPlugin', () => { let server: FakeServer; let broadcastSpy: jest.SpyInstance; const url = 'http://example.com/plugin'; + const SyncRTLPluginStateMessageName = 'syncRTLPluginState'; beforeEach(() => { server = fakeServer.create(); global.fetch = null; diff --git a/src/source/rtl_text_plugin_main_thread.ts b/src/source/rtl_text_plugin_main_thread.ts index 831943489b..b64eb7face 100644 --- a/src/source/rtl_text_plugin_main_thread.ts +++ b/src/source/rtl_text_plugin_main_thread.ts @@ -5,7 +5,6 @@ import {RTLPluginStatus, RTLPluginLoadedEventName, PluginState} from './rtl_text import {Dispatcher, getGlobalDispatcher} from '../util/dispatcher'; import {getArrayBuffer} from '../util/ajax'; -import {SyncRTLPluginStateMessageName} from '../util/actor_messages'; import {WorkerPool} from '../util/worker_pool'; class RTLMainThreadPlugin extends Evented { @@ -16,7 +15,7 @@ class RTLMainThreadPlugin extends Evented { /** Sync RTL plugin state by broadcasting a message to the worker */ _syncState(statusToSend: RTLPluginStatus): Promise { this.status = statusToSend; - return this.dispatcher.broadcast(SyncRTLPluginStateMessageName, {pluginStatus: statusToSend, pluginURL: this.url}); + return this.dispatcher.broadcast('syncRTLPluginState', {pluginStatus: statusToSend, pluginURL: this.url}); } /** This one is exposed to outside */ @@ -72,21 +71,19 @@ class RTLMainThreadPlugin extends Evented { try { const workerResults = await this._syncState('loading'); - if (workerResults.length > 0) { - - // expect all of them to be 'loaded' - const expectedStatus = 'loaded'; - const failedToLoadWorkers = workerResults.filter((workerResult) => { - return workerResult.pluginStatus !== expectedStatus; - }); - - if (failedToLoadWorkers.length > 0) { - throw failedToLoadWorkers[0].pluginStatus; - } else { - // all success - this.status = expectedStatus; - this.fire(new Event(RTLPluginLoadedEventName)); - } + + // expect all of them to be 'loaded' + const expectedStatus = 'loaded'; + const failedToLoadWorkers = workerResults.filter((workerResult) => { + return workerResult.pluginStatus !== expectedStatus; + }); + + if (failedToLoadWorkers.length > 0) { + throw failedToLoadWorkers[0].pluginStatus; + } else { + // all success + this.status = expectedStatus; + this.fire(new Event(RTLPluginLoadedEventName)); } } catch (e) { this.status = 'error'; diff --git a/src/source/rtl_text_plugin_worker.ts b/src/source/rtl_text_plugin_worker.ts index c4c34a7e92..88f8d2187e 100644 --- a/src/source/rtl_text_plugin_worker.ts +++ b/src/source/rtl_text_plugin_worker.ts @@ -18,6 +18,13 @@ class RTLWorkerPlugin implements RTLTextPlugin { this.pluginURL = state.pluginURL; } + getState(): PluginState { + return { + pluginStatus: this.pluginStatus, + pluginURL: this.pluginURL + }; + } + setMethods(rtlTextPlugin: RTLTextPlugin) { this.applyArabicShaping = rtlTextPlugin.applyArabicShaping; this.processBidirectionalText = rtlTextPlugin.processBidirectionalText; diff --git a/src/source/worker.test.ts b/src/source/worker.test.ts index de96e61c42..45bb72495f 100644 --- a/src/source/worker.test.ts +++ b/src/source/worker.test.ts @@ -7,7 +7,6 @@ import {WorkerSource, WorkerTileParameters, WorkerTileResult} from './worker_sou import {rtlWorkerPlugin} from './rtl_text_plugin_worker'; import {ActorTarget, IActor} from '../util/actor'; import {PluginState} from './rtl_text_plugin_status'; -import {SyncRTLPluginStateMessageName} from '../util/actor_messages'; class WorkerSourceMock implements WorkerSource { availableImages: string[]; @@ -87,7 +86,7 @@ describe('Worker RTLTextPlugin', () => { pluginStatus: 'deferred' }; - await worker.actor.messageHandlers[SyncRTLPluginStateMessageName]('', mockMessage); + await worker.actor.messageHandlers['syncRTLPluginState']('', mockMessage); expect(rtlWorkerPlugin.getRTLTextPluginStatus()).toBe('deferred'); }); @@ -110,7 +109,7 @@ describe('Worker RTLTextPlugin', () => { }); }); - const syncResult: PluginState = await worker.actor.messageHandlers[SyncRTLPluginStateMessageName]('', mockMessage) as any; + const syncResult: PluginState = await worker.actor.messageHandlers['syncRTLPluginState']('', mockMessage) as any; expect(rtlWorkerPlugin.getRTLTextPluginStatus()).toBe('loaded'); expect(importSpy).toHaveBeenCalledWith(mockURL); @@ -133,7 +132,7 @@ describe('Worker RTLTextPlugin', () => { pluginStatus: 'loading' }; - const workerResult: PluginState = await worker.actor.messageHandlers[SyncRTLPluginStateMessageName]('', mockMessage) as any; + const workerResult: PluginState = await worker.actor.messageHandlers['syncRTLPluginState']('', mockMessage) as any; expect(rtlWorkerPlugin.getRTLTextPluginStatus()).toBe('loaded'); expect(rtlWorkerPlugin.getPluginURL()).toBe(originalUrl); diff --git a/src/source/worker.ts b/src/source/worker.ts index 9701710004..3129802688 100644 --- a/src/source/worker.ts +++ b/src/source/worker.ts @@ -18,7 +18,6 @@ import type { import type {WorkerGlobalScopeInterface} from '../util/web_worker'; import type {LayerSpecification} from '@maplibre/maplibre-gl-style-spec'; import { - SyncRTLPluginStateMessageName, type ClusterIDAndSource, type GetClusterLeavesParams, type RemoveSourceParams, @@ -155,7 +154,7 @@ export default class Worker { this.referrer = params; }); - this.actor.registerMessageHandler(SyncRTLPluginStateMessageName, (mapId: string, params: PluginState) => { + this.actor.registerMessageHandler('syncRTLPluginState', (mapId: string, params: PluginState) => { return this._syncRTLPluginState(mapId, params); }); @@ -187,29 +186,27 @@ export default class Worker { } private async _syncRTLPluginState(mapId: string, incomingState: PluginState): Promise { - const resultState = incomingState; // Parsed plugin cannot be changed, so just return its current state. if (rtlWorkerPlugin.isParsed()) { - return { - pluginURL: rtlWorkerPlugin.getPluginURL(), - pluginStatus: rtlWorkerPlugin.getRTLTextPluginStatus() - }; + return rtlWorkerPlugin.getState(); } if (incomingState.pluginStatus !== 'loading') { // simply sync and done - rtlWorkerPlugin.setState(resultState); - return resultState; + rtlWorkerPlugin.setState(incomingState); + return incomingState; } const urlToLoad = incomingState.pluginURL; this.self.importScripts(urlToLoad); const complete = rtlWorkerPlugin.isParsed(); if (complete) { - resultState.pluginStatus = 'loaded'; - resultState.pluginURL = urlToLoad; - rtlWorkerPlugin.setState(resultState); - return resultState; + const loadedState: PluginState = { + pluginStatus: 'loaded', + pluginURL: urlToLoad + }; + rtlWorkerPlugin.setState(loadedState); + return loadedState; } // error case diff --git a/src/util/actor_messages.ts b/src/util/actor_messages.ts index d8530bc9cc..4a3be8cd8e 100644 --- a/src/util/actor_messages.ts +++ b/src/util/actor_messages.ts @@ -80,9 +80,6 @@ export type GetGlyphsResponse = { */ export type GetImagesResponse = {[_: string]: StyleImage} -/** the message name to sync RTL Plugin state. Shared by main thread and worker */ -export const SyncRTLPluginStateMessageName = 'syncRTLPluginState'; - /** * This is basically a mapping between all the calls that are made to and from the workers. * The key is the event name, the first parameter is the event input type, and the last parameter is the output type. @@ -100,7 +97,7 @@ export type RequestResponseMessageMap = { 'setImages': [string[], void]; 'setLayers': [Array, void]; 'updateLayers': [UpdateLayersParamaeters, void]; - [SyncRTLPluginStateMessageName]: [PluginState, PluginState]; + 'syncRTLPluginState': [PluginState, PluginState]; 'setReferrer': [string, void]; 'removeSource': [RemoveSourceParams, void]; 'removeMap': [undefined, void]; From b039280623cf9d66a89c1a7bc6465023d52bc045 Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Wed, 6 Mar 2024 12:08:29 -0800 Subject: [PATCH 17/30] replace exception with console.error --- src/source/rtl_text_plugin_main_thread.test.ts | 16 +++++++++++----- src/source/rtl_text_plugin_main_thread.ts | 2 +- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/source/rtl_text_plugin_main_thread.test.ts b/src/source/rtl_text_plugin_main_thread.test.ts index dee69eaf63..f5b6f38364 100644 --- a/src/source/rtl_text_plugin_main_thread.test.ts +++ b/src/source/rtl_text_plugin_main_thread.test.ts @@ -12,6 +12,7 @@ describe('RTLMainThreadPlugin', () => { let broadcastSpy: jest.SpyInstance; const url = 'http://example.com/plugin'; const SyncRTLPluginStateMessageName = 'syncRTLPluginState'; + let consoleSpy: jest.SpyInstance; beforeEach(() => { server = fakeServer.create(); global.fetch = null; @@ -64,6 +65,9 @@ describe('RTLMainThreadPlugin', () => { afterEach(() => { server.restore(); broadcastSpy.mockRestore(); + if (consoleSpy) { + consoleSpy.mockRestore(); + } }); it('should get the RTL text plugin status', () => { @@ -97,8 +101,9 @@ describe('RTLMainThreadPlugin', () => { it('should be in error state if download fails', async () => { broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockFailure as any); - const promise = rtlMainThreadPlugin.setRTLTextPlugin(url); - await expect(promise).rejects.toThrow(`worker failed to load ${url}`); + consoleSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + await rtlMainThreadPlugin.setRTLTextPlugin(url); + expect(consoleSpy).toHaveBeenCalledWith(`worker failed to load ${url}, worker status is error`); expect(rtlMainThreadPlugin.url).toEqual(url); expect(rtlMainThreadPlugin.status).toBe('error'); }); @@ -155,9 +160,10 @@ describe('RTLMainThreadPlugin', () => { expect(rtlMainThreadPlugin.status).toBe('requested'); }); - it('should throw error for mixed success and failure', async () => { + it('should report error for multiple results and one failure', async () => { broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockMix as any); - const promise = rtlMainThreadPlugin.setRTLTextPlugin(url); - await expect(promise).rejects.toThrow(`worker failed to load ${url}, worker status is error`); + consoleSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + await rtlMainThreadPlugin.setRTLTextPlugin(url); + expect(consoleSpy).toHaveBeenCalledWith(`worker failed to load ${url}, worker status is error`); }); }); diff --git a/src/source/rtl_text_plugin_main_thread.ts b/src/source/rtl_text_plugin_main_thread.ts index b64eb7face..20d0c9740e 100644 --- a/src/source/rtl_text_plugin_main_thread.ts +++ b/src/source/rtl_text_plugin_main_thread.ts @@ -87,7 +87,7 @@ class RTLMainThreadPlugin extends Evented { } } catch (e) { this.status = 'error'; - throw new Error(`worker failed to load ${this.url}, worker status is ${e.toString()}`); + console.error(`worker failed to load ${this.url}, worker status is ${e.toString()}`); } } From 1cf03dda2b3f953eb47e3739a10acffc43d0e216 Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Wed, 6 Mar 2024 12:15:42 -0800 Subject: [PATCH 18/30] update comment to be more accurate --- src/data/bucket/symbol_bucket.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/data/bucket/symbol_bucket.ts b/src/data/bucket/symbol_bucket.ts index 545c16d7ac..befc030c4f 100644 --- a/src/data/bucket/symbol_bucket.ts +++ b/src/data/bucket/symbol_bucket.ts @@ -483,7 +483,7 @@ export class SymbolBucket implements Bucket { const resolvedTokens = layer.getValueAndResolveTokens('text-field', evaluationFeature, canonical, availableImages); const formattedText = Formatted.factory(resolvedTokens); - // hasRTLText only needs to be checked once per bucket + // on this instance: if hasRTLText is already true, all future calls to containsRTLText can be skipped. const bucketHasRTLText = this.hasRTLText = (this.hasRTLText || containsRTLText(formattedText)); if ( !bucketHasRTLText || // non-rtl text so can proceed safely From 44d2b845161e095e88e471376295bd80b05def12 Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Thu, 7 Mar 2024 11:52:38 -0800 Subject: [PATCH 19/30] do not download from main thread --- src/source/rtl_text_plugin_main_thread.ts | 15 +++------------ src/source/worker.ts | 4 ++++ 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/src/source/rtl_text_plugin_main_thread.ts b/src/source/rtl_text_plugin_main_thread.ts index 20d0c9740e..a6c994d2c9 100644 --- a/src/source/rtl_text_plugin_main_thread.ts +++ b/src/source/rtl_text_plugin_main_thread.ts @@ -60,26 +60,17 @@ class RTLMainThreadPlugin extends Evented { /** Send a message to worker which will import the RTL plugin script */ async _requestImport() : Promise { - - // Reference PR: https://github.com/mapbox/mapbox-gl-js/pull/9122 - // if have more than 1 workers, it is better to load once in main thread to warm up browser cache - // so all workers can use it --- even though the result of getArrayBuffer is not being used here. - // Otherwise, just let worker importScript once. - if (WorkerPool.workerCount > 1) { - await getArrayBuffer({url: this.url}, new AbortController()); - } - try { const workerResults = await this._syncState('loading'); // expect all of them to be 'loaded' const expectedStatus = 'loaded'; - const failedToLoadWorkers = workerResults.filter((workerResult) => { + const failedToLoadWorker = workerResults.find((workerResult) => { return workerResult.pluginStatus !== expectedStatus; }); - if (failedToLoadWorkers.length > 0) { - throw failedToLoadWorkers[0].pluginStatus; + if (failedToLoadWorker) { + throw failedToLoadWorker.pluginStatus; } else { // all success this.status = expectedStatus; diff --git a/src/source/worker.ts b/src/source/worker.ts index 3129802688..c5cf33cf6e 100644 --- a/src/source/worker.ts +++ b/src/source/worker.ts @@ -210,6 +210,10 @@ export default class Worker { } // error case + rtlWorkerPlugin.setState({ + pluginStatus: 'error', + pluginURL: '' + }); throw new Error(`RTL Text Plugin failed to import scripts from ${urlToLoad}`); } From f725ac60dfcc4e71043fcd0b54f65527b1551758 Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Thu, 7 Mar 2024 13:09:54 -0800 Subject: [PATCH 20/30] remove extra try/catch --- .../rtl_text_plugin_main_thread.test.ts | 26 +++++++++-------- src/source/rtl_text_plugin_main_thread.ts | 28 ++++++++----------- 2 files changed, 26 insertions(+), 28 deletions(-) diff --git a/src/source/rtl_text_plugin_main_thread.test.ts b/src/source/rtl_text_plugin_main_thread.test.ts index f5b6f38364..e705012e30 100644 --- a/src/source/rtl_text_plugin_main_thread.test.ts +++ b/src/source/rtl_text_plugin_main_thread.test.ts @@ -12,7 +12,7 @@ describe('RTLMainThreadPlugin', () => { let broadcastSpy: jest.SpyInstance; const url = 'http://example.com/plugin'; const SyncRTLPluginStateMessageName = 'syncRTLPluginState'; - let consoleSpy: jest.SpyInstance; + beforeEach(() => { server = fakeServer.create(); global.fetch = null; @@ -65,9 +65,6 @@ describe('RTLMainThreadPlugin', () => { afterEach(() => { server.restore(); broadcastSpy.mockRestore(); - if (consoleSpy) { - consoleSpy.mockRestore(); - } }); it('should get the RTL text plugin status', () => { @@ -101,11 +98,12 @@ describe('RTLMainThreadPlugin', () => { it('should be in error state if download fails', async () => { broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockFailure as any); - consoleSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); - await rtlMainThreadPlugin.setRTLTextPlugin(url); - expect(consoleSpy).toHaveBeenCalledWith(`worker failed to load ${url}, worker status is error`); - expect(rtlMainThreadPlugin.url).toEqual(url); - expect(rtlMainThreadPlugin.status).toBe('error'); + try { + await rtlMainThreadPlugin.setRTLTextPlugin(url); + } catch { + expect(rtlMainThreadPlugin.url).toEqual(url); + expect(rtlMainThreadPlugin.status).toBe('error'); + } }); it('should lazy load the plugin if deferred', async () => { @@ -162,8 +160,12 @@ describe('RTLMainThreadPlugin', () => { it('should report error for multiple results and one failure', async () => { broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockMix as any); - consoleSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); - await rtlMainThreadPlugin.setRTLTextPlugin(url); - expect(consoleSpy).toHaveBeenCalledWith(`worker failed to load ${url}, worker status is error`); + try { + const promise = rtlMainThreadPlugin.setRTLTextPlugin(url); + await expect(promise).rejects.toThrow(`worker failed to load ${url}`); + } catch { + expect(rtlMainThreadPlugin.url).toEqual(url); + expect(rtlMainThreadPlugin.status).toBe('error'); + } }); }); diff --git a/src/source/rtl_text_plugin_main_thread.ts b/src/source/rtl_text_plugin_main_thread.ts index a6c994d2c9..028e58bfba 100644 --- a/src/source/rtl_text_plugin_main_thread.ts +++ b/src/source/rtl_text_plugin_main_thread.ts @@ -60,25 +60,21 @@ class RTLMainThreadPlugin extends Evented { /** Send a message to worker which will import the RTL plugin script */ async _requestImport() : Promise { - try { - const workerResults = await this._syncState('loading'); + const workerResults = await this._syncState('loading'); - // expect all of them to be 'loaded' - const expectedStatus = 'loaded'; - const failedToLoadWorker = workerResults.find((workerResult) => { - return workerResult.pluginStatus !== expectedStatus; - }); + // expect all of them to be 'loaded' + const expectedStatus = 'loaded'; + const failedToLoadWorker = workerResults.find((workerResult) => { + return workerResult.pluginStatus !== expectedStatus; + }); - if (failedToLoadWorker) { - throw failedToLoadWorker.pluginStatus; - } else { - // all success - this.status = expectedStatus; - this.fire(new Event(RTLPluginLoadedEventName)); - } - } catch (e) { + if (failedToLoadWorker) { this.status = 'error'; - console.error(`worker failed to load ${this.url}, worker status is ${e.toString()}`); + throw failedToLoadWorker.pluginStatus; + } else { + // all success + this.status = expectedStatus; + this.fire(new Event(RTLPluginLoadedEventName)); } } From f46db6ca3a929064a43c059e4a03aefe8cb5ad38 Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Thu, 7 Mar 2024 13:20:35 -0800 Subject: [PATCH 21/30] remove extra throw --- .../rtl_text_plugin_main_thread.test.ts | 22 ++++++++----------- src/source/rtl_text_plugin_main_thread.ts | 5 ++--- 2 files changed, 11 insertions(+), 16 deletions(-) diff --git a/src/source/rtl_text_plugin_main_thread.test.ts b/src/source/rtl_text_plugin_main_thread.test.ts index e705012e30..5fda93ec3c 100644 --- a/src/source/rtl_text_plugin_main_thread.test.ts +++ b/src/source/rtl_text_plugin_main_thread.test.ts @@ -98,12 +98,9 @@ describe('RTLMainThreadPlugin', () => { it('should be in error state if download fails', async () => { broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockFailure as any); - try { - await rtlMainThreadPlugin.setRTLTextPlugin(url); - } catch { - expect(rtlMainThreadPlugin.url).toEqual(url); - expect(rtlMainThreadPlugin.status).toBe('error'); - } + await rtlMainThreadPlugin.setRTLTextPlugin(url); + expect(rtlMainThreadPlugin.url).toEqual(url); + expect(rtlMainThreadPlugin.status).toBe('error'); }); it('should lazy load the plugin if deferred', async () => { @@ -160,12 +157,11 @@ describe('RTLMainThreadPlugin', () => { it('should report error for multiple results and one failure', async () => { broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockMix as any); - try { - const promise = rtlMainThreadPlugin.setRTLTextPlugin(url); - await expect(promise).rejects.toThrow(`worker failed to load ${url}`); - } catch { - expect(rtlMainThreadPlugin.url).toEqual(url); - expect(rtlMainThreadPlugin.status).toBe('error'); - } + + await rtlMainThreadPlugin.setRTLTextPlugin(url); + + expect(rtlMainThreadPlugin.url).toEqual(url); + expect(rtlMainThreadPlugin.status).toBe('error'); + }); }); diff --git a/src/source/rtl_text_plugin_main_thread.ts b/src/source/rtl_text_plugin_main_thread.ts index 028e58bfba..778f939f86 100644 --- a/src/source/rtl_text_plugin_main_thread.ts +++ b/src/source/rtl_text_plugin_main_thread.ts @@ -64,13 +64,12 @@ class RTLMainThreadPlugin extends Evented { // expect all of them to be 'loaded' const expectedStatus = 'loaded'; - const failedToLoadWorker = workerResults.find((workerResult) => { + const failedToLoadWorkerExist: boolean = workerResults.some((workerResult) => { return workerResult.pluginStatus !== expectedStatus; }); - if (failedToLoadWorker) { + if (failedToLoadWorkerExist) { this.status = 'error'; - throw failedToLoadWorker.pluginStatus; } else { // all success this.status = expectedStatus; From c15def2219d0e294338b5f17f47827d8193bfa0a Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Thu, 7 Mar 2024 13:21:53 -0800 Subject: [PATCH 22/30] remove extra import --- src/source/rtl_text_plugin_main_thread.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/source/rtl_text_plugin_main_thread.ts b/src/source/rtl_text_plugin_main_thread.ts index 778f939f86..7e9b62e2bf 100644 --- a/src/source/rtl_text_plugin_main_thread.ts +++ b/src/source/rtl_text_plugin_main_thread.ts @@ -4,9 +4,6 @@ import {Event, Evented} from '../util/evented'; import {RTLPluginStatus, RTLPluginLoadedEventName, PluginState} from './rtl_text_plugin_status'; import {Dispatcher, getGlobalDispatcher} from '../util/dispatcher'; -import {getArrayBuffer} from '../util/ajax'; -import {WorkerPool} from '../util/worker_pool'; - class RTLMainThreadPlugin extends Evented { status: RTLPluginStatus = 'unavailable'; url: string = null; From 0ca962c1b2a01a7ed2449918a0fbb47008fe1573 Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Mon, 11 Mar 2024 16:07:33 -0700 Subject: [PATCH 23/30] exception handling --- src/source/rtl_text_plugin_main_thread.ts | 25 +++++++++-------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/src/source/rtl_text_plugin_main_thread.ts b/src/source/rtl_text_plugin_main_thread.ts index 7e9b62e2bf..c350addc92 100644 --- a/src/source/rtl_text_plugin_main_thread.ts +++ b/src/source/rtl_text_plugin_main_thread.ts @@ -12,7 +12,12 @@ class RTLMainThreadPlugin extends Evented { /** Sync RTL plugin state by broadcasting a message to the worker */ _syncState(statusToSend: RTLPluginStatus): Promise { this.status = statusToSend; - return this.dispatcher.broadcast('syncRTLPluginState', {pluginStatus: statusToSend, pluginURL: this.url}); + try { + return this.dispatcher.broadcast('syncRTLPluginState', {pluginStatus: statusToSend, pluginURL: this.url}); + } catch (e) { + this.status = 'error'; + throw e; + } } /** This one is exposed to outside */ @@ -57,21 +62,11 @@ class RTLMainThreadPlugin extends Evented { /** Send a message to worker which will import the RTL plugin script */ async _requestImport() : Promise { - const workerResults = await this._syncState('loading'); - - // expect all of them to be 'loaded' - const expectedStatus = 'loaded'; - const failedToLoadWorkerExist: boolean = workerResults.some((workerResult) => { - return workerResult.pluginStatus !== expectedStatus; - }); - if (failedToLoadWorkerExist) { - this.status = 'error'; - } else { - // all success - this.status = expectedStatus; - this.fire(new Event(RTLPluginLoadedEventName)); - } + // all errors/exceptions will be handled by _syncState + await this._syncState('loading'); + this.status = 'loaded'; + this.fire(new Event(RTLPluginLoadedEventName)); } /** Start a lazy loading process of RTL plugin */ From 191192c676d39bcaafc6e42249822a58c0889eb8 Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Mon, 11 Mar 2024 16:11:13 -0700 Subject: [PATCH 24/30] correct version --- src/source/rtl_text_plugin_main_thread.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/source/rtl_text_plugin_main_thread.ts b/src/source/rtl_text_plugin_main_thread.ts index c350addc92..95c9a7a1ca 100644 --- a/src/source/rtl_text_plugin_main_thread.ts +++ b/src/source/rtl_text_plugin_main_thread.ts @@ -12,12 +12,11 @@ class RTLMainThreadPlugin extends Evented { /** Sync RTL plugin state by broadcasting a message to the worker */ _syncState(statusToSend: RTLPluginStatus): Promise { this.status = statusToSend; - try { - return this.dispatcher.broadcast('syncRTLPluginState', {pluginStatus: statusToSend, pluginURL: this.url}); - } catch (e) { - this.status = 'error'; - throw e; - } + return this.dispatcher.broadcast('syncRTLPluginState', {pluginStatus: statusToSend, pluginURL: this.url}) + .catch((e: any) => { + this.status = 'error'; + throw e; + }); } /** This one is exposed to outside */ From 3ad1e7174cab8c67783c700507a4416542f179ba Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Tue, 12 Mar 2024 13:22:54 -0700 Subject: [PATCH 25/30] all UT --- .../rtl_text_plugin_main_thread.test.ts | 40 ++++--------------- src/source/rtl_text_plugin_main_thread.ts | 14 ++++--- 2 files changed, 15 insertions(+), 39 deletions(-) diff --git a/src/source/rtl_text_plugin_main_thread.test.ts b/src/source/rtl_text_plugin_main_thread.test.ts index 5fda93ec3c..7dd95832be 100644 --- a/src/source/rtl_text_plugin_main_thread.test.ts +++ b/src/source/rtl_text_plugin_main_thread.test.ts @@ -11,6 +11,7 @@ describe('RTLMainThreadPlugin', () => { let server: FakeServer; let broadcastSpy: jest.SpyInstance; const url = 'http://example.com/plugin'; + const failedToLoadMessage = `RTL Text Plugin failed to import scripts from ${url}`; const SyncRTLPluginStateMessageName = 'syncRTLPluginState'; beforeEach(() => { @@ -36,29 +37,10 @@ describe('RTLMainThreadPlugin', () => { function broadcastMockFailure(message: MessageType, payload: PluginState): Promise { if (message === SyncRTLPluginStateMessageName) { if (payload.pluginStatus === 'loading') { - const resultState: PluginState = { - pluginStatus: 'error', - pluginURL: payload.pluginURL - }; - return Promise.resolve([resultState]); - } - } - } - - /** return two results, one success one failure */ - function broadcastMockMix(message: MessageType, payload: PluginState): Promise { - if (message === SyncRTLPluginStateMessageName) { - if (payload.pluginStatus === 'loading') { - const resultState0: PluginState = { - pluginStatus: 'loaded', - pluginURL: payload.pluginURL - }; - const resultState1: PluginState = { - pluginStatus: 'error', - pluginURL: payload.pluginURL - }; - return Promise.resolve([resultState0, resultState1]); + throw new Error(failedToLoadMessage); } + } else { + return Promise.resolve([]); } } @@ -98,7 +80,9 @@ describe('RTLMainThreadPlugin', () => { it('should be in error state if download fails', async () => { broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockFailure as any); - await rtlMainThreadPlugin.setRTLTextPlugin(url); + const result = rtlMainThreadPlugin.setRTLTextPlugin(url); + + await expect(result).rejects.toThrow(failedToLoadMessage); expect(rtlMainThreadPlugin.url).toEqual(url); expect(rtlMainThreadPlugin.status).toBe('error'); }); @@ -154,14 +138,4 @@ describe('RTLMainThreadPlugin', () => { rtlMainThreadPlugin.lazyLoad(); expect(rtlMainThreadPlugin.status).toBe('requested'); }); - - it('should report error for multiple results and one failure', async () => { - broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockMix as any); - - await rtlMainThreadPlugin.setRTLTextPlugin(url); - - expect(rtlMainThreadPlugin.url).toEqual(url); - expect(rtlMainThreadPlugin.status).toBe('error'); - - }); }); diff --git a/src/source/rtl_text_plugin_main_thread.ts b/src/source/rtl_text_plugin_main_thread.ts index 95c9a7a1ca..c91e81d2c0 100644 --- a/src/source/rtl_text_plugin_main_thread.ts +++ b/src/source/rtl_text_plugin_main_thread.ts @@ -11,12 +11,13 @@ class RTLMainThreadPlugin extends Evented { /** Sync RTL plugin state by broadcasting a message to the worker */ _syncState(statusToSend: RTLPluginStatus): Promise { - this.status = statusToSend; - return this.dispatcher.broadcast('syncRTLPluginState', {pluginStatus: statusToSend, pluginURL: this.url}) - .catch((e: any) => { - this.status = 'error'; - throw e; - }); + try { + this.status = statusToSend; + return this.dispatcher.broadcast('syncRTLPluginState', {pluginStatus: statusToSend, pluginURL: this.url}); + } catch (e) { + this.status = 'error'; + throw e; + } } /** This one is exposed to outside */ @@ -64,6 +65,7 @@ class RTLMainThreadPlugin extends Evented { // all errors/exceptions will be handled by _syncState await this._syncState('loading'); + this.status = 'loaded'; this.fire(new Event(RTLPluginLoadedEventName)); } From 403d33f6a96f7ed87f9fbafc24b83c27fcc3a791 Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Tue, 12 Mar 2024 17:02:50 -0700 Subject: [PATCH 26/30] UT --- .../rtl_text_plugin_main_thread.test.ts | 53 +++++++++++++++---- src/source/rtl_text_plugin_main_thread.ts | 14 +++-- 2 files changed, 48 insertions(+), 19 deletions(-) diff --git a/src/source/rtl_text_plugin_main_thread.test.ts b/src/source/rtl_text_plugin_main_thread.test.ts index 7dd95832be..e38d78c418 100644 --- a/src/source/rtl_text_plugin_main_thread.test.ts +++ b/src/source/rtl_text_plugin_main_thread.test.ts @@ -23,6 +23,7 @@ describe('RTLMainThreadPlugin', () => { }); function broadcastMockSuccess(message: MessageType, payload: PluginState): Promise { + console.log('broadcastMockSuccessDefer', payload.pluginStatus); if (message === SyncRTLPluginStateMessageName) { if (payload.pluginStatus === 'loading') { const resultState: PluginState = { @@ -34,10 +35,22 @@ describe('RTLMainThreadPlugin', () => { } } + function broadcastMockSuccessDefer(message: MessageType, payload: PluginState): Promise { + if (message === SyncRTLPluginStateMessageName) { + if (payload.pluginStatus === 'deferred') { + const resultState: PluginState = { + pluginStatus: 'deferred', + pluginURL: payload.pluginURL + }; + return Promise.resolve([resultState]); + } + } + } + function broadcastMockFailure(message: MessageType, payload: PluginState): Promise { if (message === SyncRTLPluginStateMessageName) { if (payload.pluginStatus === 'loading') { - throw new Error(failedToLoadMessage); + return Promise.reject(failedToLoadMessage); } } else { return Promise.resolve([]); @@ -80,39 +93,42 @@ describe('RTLMainThreadPlugin', () => { it('should be in error state if download fails', async () => { broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockFailure as any); - const result = rtlMainThreadPlugin.setRTLTextPlugin(url); - - await expect(result).rejects.toThrow(failedToLoadMessage); + const resultPromise = rtlMainThreadPlugin.setRTLTextPlugin(url); + await expect(resultPromise).rejects.toBe(failedToLoadMessage); expect(rtlMainThreadPlugin.url).toEqual(url); expect(rtlMainThreadPlugin.status).toBe('error'); }); it('should lazy load the plugin if deferred', async () => { // use success spy to make sure test case does not throw exception - broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockSuccess as any); + const deferredSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockSuccessDefer as any); await rtlMainThreadPlugin.setRTLTextPlugin(url, true); - expect(broadcastSpy).toHaveBeenCalledWith(SyncRTLPluginStateMessageName, {pluginStatus: 'deferred', pluginURL: url}); + expect(deferredSpy).toHaveBeenCalledTimes(1); + expect(deferredSpy).toHaveBeenCalledWith(SyncRTLPluginStateMessageName, {pluginStatus: 'deferred', pluginURL: url}); expect(rtlMainThreadPlugin.status).toBe('deferred'); + deferredSpy.mockRestore(); // this is really a fire and forget // two calls to lazyLoad + broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockSuccess as any); rtlMainThreadPlugin.lazyLoad(); await sleep(1); expect(broadcastSpy).toHaveBeenCalledWith(SyncRTLPluginStateMessageName, {pluginStatus: 'loading', pluginURL: url}); - // two times, first for "deferred", second for 'loading' - expect(broadcastSpy).toHaveBeenCalledTimes(2); + // 'loading' + expect(broadcastSpy).toHaveBeenCalledWith(SyncRTLPluginStateMessageName, {pluginStatus: 'loading', pluginURL: url}); + expect(broadcastSpy).toHaveBeenCalledTimes(1); - // second call to lazyLoad should not change anything + // // second call to lazyLoad should not change anything rtlMainThreadPlugin.lazyLoad(); - expect(broadcastSpy).toHaveBeenCalledTimes(2); + expect(broadcastSpy).toHaveBeenCalledTimes(1); expect(rtlMainThreadPlugin.status).toBe('loaded'); // 3rd call to lazyLoad should not change anything rtlMainThreadPlugin.lazyLoad(); expect(rtlMainThreadPlugin.status).toBe('loaded'); - expect(broadcastSpy).toHaveBeenCalledTimes(2); + expect(broadcastSpy).toHaveBeenCalledTimes(1); }); it('should set status to requested if RTL plugin was not set', async () => { @@ -138,4 +154,19 @@ describe('RTLMainThreadPlugin', () => { rtlMainThreadPlugin.lazyLoad(); expect(rtlMainThreadPlugin.status).toBe('requested'); }); + + it('should be in error state if lazyLoad fails', async () => { + broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockSuccessDefer); + const resultPromise = rtlMainThreadPlugin.setRTLTextPlugin(url, true); + await expect(resultPromise).resolves.toBeUndefined(); + + expect(rtlMainThreadPlugin.status).toBe('deferred'); + + // the next one should fail + broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockFailure as any); + + await expect(rtlMainThreadPlugin._requestImport()).rejects.toBe(failedToLoadMessage); + expect(rtlMainThreadPlugin.url).toEqual(url); + expect(rtlMainThreadPlugin.status).toBe('error'); + }); }); diff --git a/src/source/rtl_text_plugin_main_thread.ts b/src/source/rtl_text_plugin_main_thread.ts index c91e81d2c0..95c9a7a1ca 100644 --- a/src/source/rtl_text_plugin_main_thread.ts +++ b/src/source/rtl_text_plugin_main_thread.ts @@ -11,13 +11,12 @@ class RTLMainThreadPlugin extends Evented { /** Sync RTL plugin state by broadcasting a message to the worker */ _syncState(statusToSend: RTLPluginStatus): Promise { - try { - this.status = statusToSend; - return this.dispatcher.broadcast('syncRTLPluginState', {pluginStatus: statusToSend, pluginURL: this.url}); - } catch (e) { - this.status = 'error'; - throw e; - } + this.status = statusToSend; + return this.dispatcher.broadcast('syncRTLPluginState', {pluginStatus: statusToSend, pluginURL: this.url}) + .catch((e: any) => { + this.status = 'error'; + throw e; + }); } /** This one is exposed to outside */ @@ -65,7 +64,6 @@ class RTLMainThreadPlugin extends Evented { // all errors/exceptions will be handled by _syncState await this._syncState('loading'); - this.status = 'loaded'; this.fire(new Event(RTLPluginLoadedEventName)); } From e49229d6b80ae612dfea40a76662b20991927769 Mon Sep 17 00:00:00 2001 From: zhangyiatmicrosoft <37553549+zhangyiatmicrosoft@users.noreply.github.com> Date: Tue, 12 Mar 2024 17:47:19 -0700 Subject: [PATCH 27/30] add tests (#7) * all UT * UT * Add FT --- .../rtl_text_plugin_main_thread.test.ts | 57 ++++++++++--------- test/integration/browser/browser.test.ts | 19 ++++++- 2 files changed, 49 insertions(+), 27 deletions(-) diff --git a/src/source/rtl_text_plugin_main_thread.test.ts b/src/source/rtl_text_plugin_main_thread.test.ts index 5fda93ec3c..e38d78c418 100644 --- a/src/source/rtl_text_plugin_main_thread.test.ts +++ b/src/source/rtl_text_plugin_main_thread.test.ts @@ -11,6 +11,7 @@ describe('RTLMainThreadPlugin', () => { let server: FakeServer; let broadcastSpy: jest.SpyInstance; const url = 'http://example.com/plugin'; + const failedToLoadMessage = `RTL Text Plugin failed to import scripts from ${url}`; const SyncRTLPluginStateMessageName = 'syncRTLPluginState'; beforeEach(() => { @@ -22,6 +23,7 @@ describe('RTLMainThreadPlugin', () => { }); function broadcastMockSuccess(message: MessageType, payload: PluginState): Promise { + console.log('broadcastMockSuccessDefer', payload.pluginStatus); if (message === SyncRTLPluginStateMessageName) { if (payload.pluginStatus === 'loading') { const resultState: PluginState = { @@ -33,11 +35,11 @@ describe('RTLMainThreadPlugin', () => { } } - function broadcastMockFailure(message: MessageType, payload: PluginState): Promise { + function broadcastMockSuccessDefer(message: MessageType, payload: PluginState): Promise { if (message === SyncRTLPluginStateMessageName) { - if (payload.pluginStatus === 'loading') { + if (payload.pluginStatus === 'deferred') { const resultState: PluginState = { - pluginStatus: 'error', + pluginStatus: 'deferred', pluginURL: payload.pluginURL }; return Promise.resolve([resultState]); @@ -45,20 +47,13 @@ describe('RTLMainThreadPlugin', () => { } } - /** return two results, one success one failure */ - function broadcastMockMix(message: MessageType, payload: PluginState): Promise { + function broadcastMockFailure(message: MessageType, payload: PluginState): Promise { if (message === SyncRTLPluginStateMessageName) { if (payload.pluginStatus === 'loading') { - const resultState0: PluginState = { - pluginStatus: 'loaded', - pluginURL: payload.pluginURL - }; - const resultState1: PluginState = { - pluginStatus: 'error', - pluginURL: payload.pluginURL - }; - return Promise.resolve([resultState0, resultState1]); + return Promise.reject(failedToLoadMessage); } + } else { + return Promise.resolve([]); } } @@ -98,37 +93,42 @@ describe('RTLMainThreadPlugin', () => { it('should be in error state if download fails', async () => { broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockFailure as any); - await rtlMainThreadPlugin.setRTLTextPlugin(url); + const resultPromise = rtlMainThreadPlugin.setRTLTextPlugin(url); + await expect(resultPromise).rejects.toBe(failedToLoadMessage); expect(rtlMainThreadPlugin.url).toEqual(url); expect(rtlMainThreadPlugin.status).toBe('error'); }); it('should lazy load the plugin if deferred', async () => { // use success spy to make sure test case does not throw exception - broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockSuccess as any); + const deferredSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockSuccessDefer as any); await rtlMainThreadPlugin.setRTLTextPlugin(url, true); - expect(broadcastSpy).toHaveBeenCalledWith(SyncRTLPluginStateMessageName, {pluginStatus: 'deferred', pluginURL: url}); + expect(deferredSpy).toHaveBeenCalledTimes(1); + expect(deferredSpy).toHaveBeenCalledWith(SyncRTLPluginStateMessageName, {pluginStatus: 'deferred', pluginURL: url}); expect(rtlMainThreadPlugin.status).toBe('deferred'); + deferredSpy.mockRestore(); // this is really a fire and forget // two calls to lazyLoad + broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockSuccess as any); rtlMainThreadPlugin.lazyLoad(); await sleep(1); expect(broadcastSpy).toHaveBeenCalledWith(SyncRTLPluginStateMessageName, {pluginStatus: 'loading', pluginURL: url}); - // two times, first for "deferred", second for 'loading' - expect(broadcastSpy).toHaveBeenCalledTimes(2); + // 'loading' + expect(broadcastSpy).toHaveBeenCalledWith(SyncRTLPluginStateMessageName, {pluginStatus: 'loading', pluginURL: url}); + expect(broadcastSpy).toHaveBeenCalledTimes(1); - // second call to lazyLoad should not change anything + // // second call to lazyLoad should not change anything rtlMainThreadPlugin.lazyLoad(); - expect(broadcastSpy).toHaveBeenCalledTimes(2); + expect(broadcastSpy).toHaveBeenCalledTimes(1); expect(rtlMainThreadPlugin.status).toBe('loaded'); // 3rd call to lazyLoad should not change anything rtlMainThreadPlugin.lazyLoad(); expect(rtlMainThreadPlugin.status).toBe('loaded'); - expect(broadcastSpy).toHaveBeenCalledTimes(2); + expect(broadcastSpy).toHaveBeenCalledTimes(1); }); it('should set status to requested if RTL plugin was not set', async () => { @@ -155,13 +155,18 @@ describe('RTLMainThreadPlugin', () => { expect(rtlMainThreadPlugin.status).toBe('requested'); }); - it('should report error for multiple results and one failure', async () => { - broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockMix as any); + it('should be in error state if lazyLoad fails', async () => { + broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockSuccessDefer); + const resultPromise = rtlMainThreadPlugin.setRTLTextPlugin(url, true); + await expect(resultPromise).resolves.toBeUndefined(); - await rtlMainThreadPlugin.setRTLTextPlugin(url); + expect(rtlMainThreadPlugin.status).toBe('deferred'); + + // the next one should fail + broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockFailure as any); + await expect(rtlMainThreadPlugin._requestImport()).rejects.toBe(failedToLoadMessage); expect(rtlMainThreadPlugin.url).toEqual(url); expect(rtlMainThreadPlugin.status).toBe('error'); - }); }); diff --git a/test/integration/browser/browser.test.ts b/test/integration/browser/browser.test.ts index f7d02b6c3d..bd9a2c7ea9 100644 --- a/test/integration/browser/browser.test.ts +++ b/test/integration/browser/browser.test.ts @@ -1,4 +1,4 @@ -import puppeteer, {Page, Browser} from 'puppeteer'; +import puppeteer, {Page, Browser, ConsoleMessage} from 'puppeteer'; import st from 'st'; import http, {type Server} from 'http'; import type {AddressInfo} from 'net'; @@ -385,4 +385,21 @@ describe('Browser tests', () => { expect(markerOpacity).toBe('1'); }, 20000); + + test('Load map with RTL plugin should throw exception for invalid URL', async () => { + + const rtlPromise = page.evaluate(() => { + // console.log('Testing start'); + return maplibregl.setRTLTextPlugin('badURL', false); + }); + + await rtlPromise.catch(e => { + const message: string = e.message; + + // exact message looks like + // Failed to execute 'importScripts' on 'WorkerGlobalScope': The script at 'http://localhost:52015/test/integration/browser/fixtures/badURL' failed to load. + const regex = new RegExp('Failed to execute \'importScripts\' on \'WorkerGlobalScope\': The script at \'.*\' failed to load.'); + expect(regex.test(message)).toBe(true); + }); + }, 2000); }); From 560a66c5123b600262e8625b60c6e3df63e9ac8a Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Tue, 12 Mar 2024 17:49:38 -0700 Subject: [PATCH 28/30] remove extra import --- test/integration/browser/browser.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/browser/browser.test.ts b/test/integration/browser/browser.test.ts index bd9a2c7ea9..e3dcf59429 100644 --- a/test/integration/browser/browser.test.ts +++ b/test/integration/browser/browser.test.ts @@ -1,4 +1,4 @@ -import puppeteer, {Page, Browser, ConsoleMessage} from 'puppeteer'; +import puppeteer, {Page, Browser} from 'puppeteer'; import st from 'st'; import http, {type Server} from 'http'; import type {AddressInfo} from 'net'; From 90227e4534f5a6a5597f1bad715879864cfc9f2e Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Tue, 12 Mar 2024 17:54:19 -0700 Subject: [PATCH 29/30] clean up comments --- src/source/rtl_text_plugin_main_thread.test.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/source/rtl_text_plugin_main_thread.test.ts b/src/source/rtl_text_plugin_main_thread.test.ts index e38d78c418..16d447e8f2 100644 --- a/src/source/rtl_text_plugin_main_thread.test.ts +++ b/src/source/rtl_text_plugin_main_thread.test.ts @@ -109,17 +109,15 @@ describe('RTLMainThreadPlugin', () => { deferredSpy.mockRestore(); // this is really a fire and forget - // two calls to lazyLoad broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockSuccess as any); rtlMainThreadPlugin.lazyLoad(); await sleep(1); - expect(broadcastSpy).toHaveBeenCalledWith(SyncRTLPluginStateMessageName, {pluginStatus: 'loading', pluginURL: url}); // 'loading' expect(broadcastSpy).toHaveBeenCalledWith(SyncRTLPluginStateMessageName, {pluginStatus: 'loading', pluginURL: url}); expect(broadcastSpy).toHaveBeenCalledTimes(1); - // // second call to lazyLoad should not change anything + // second call to lazyLoad should not change anything rtlMainThreadPlugin.lazyLoad(); expect(broadcastSpy).toHaveBeenCalledTimes(1); From 91e82ed15fd70d37ac0ce8107782f8efbd619dd2 Mon Sep 17 00:00:00 2001 From: Yi Zhang Date: Wed, 13 Mar 2024 09:44:59 -0700 Subject: [PATCH 30/30] test update --- test/integration/browser/browser.test.ts | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/test/integration/browser/browser.test.ts b/test/integration/browser/browser.test.ts index e3dcf59429..91aadaa6ba 100644 --- a/test/integration/browser/browser.test.ts +++ b/test/integration/browser/browser.test.ts @@ -393,13 +393,11 @@ describe('Browser tests', () => { return maplibregl.setRTLTextPlugin('badURL', false); }); - await rtlPromise.catch(e => { - const message: string = e.message; + // exact message looks like + // Failed to execute 'importScripts' on 'WorkerGlobalScope': The script at 'http://localhost:52015/test/integration/browser/fixtures/badURL' failed to load. + const regex = new RegExp('Failed to execute \'importScripts\'.*'); + + await expect(rtlPromise).rejects.toThrow(regex); - // exact message looks like - // Failed to execute 'importScripts' on 'WorkerGlobalScope': The script at 'http://localhost:52015/test/integration/browser/fixtures/badURL' failed to load. - const regex = new RegExp('Failed to execute \'importScripts\' on \'WorkerGlobalScope\': The script at \'.*\' failed to load.'); - expect(regex.test(message)).toBe(true); - }); }, 2000); });