From dc7d1f5e1099e2b12bf02390385b47502ab80ed1 Mon Sep 17 00:00:00 2001 From: Mohamed Elkmeshi Date: Fri, 4 Sep 2026 17:48:31 +0200 Subject: [PATCH] fix(maps): register the RTL text plugin so Arabic labels join MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Vector basemaps rendered Arabic, Hebrew, Persian and Urdu place names as isolated letters in logical order — "القاهرة" drawn as "ة ر ه ا ق ل ا" — because neither GL engine loads its optional RTL text plugin by default and TREK never registered one. Raster basemaps hid the problem, since their labels are drawn server-side, so it only became visible for everyone when the default basemap became a vector style. Both engine entry points now register the plugin lazily, so the payload is only fetched once a map actually paints RTL text and Latin-only users download nothing. The call is left with each engine because the two signatures genuinely differ: MapLibre takes (url, lazy) and returns a promise, Mapbox takes (url, callback, deferred). The plugin is bundled rather than loaded from a CDN: TREK is offline-first and the production CSP is script-src 'self', so a remote plugin URL would be refused by the worker that imports it. Bundling also puts it in the Workbox precache, so shaping keeps working offline. It needs a resolve alias because the package publishes only its ESM/WASM entry point through "exports" while setRTLTextPlugin requires the classic script in dist/ — the alias is shared by the build and test configs so the two cannot drift. --- client/package.json | 1 + client/rtlTextAlias.js | 25 ++++++++++ client/src/components/Map/engines/mapbox.ts | 6 +++ client/src/components/Map/engines/maplibre.ts | 6 +++ client/src/components/Map/rtlText.test.ts | 38 +++++++++++++++ client/src/components/Map/rtlText.ts | 48 +++++++++++++++++++ client/vite.config.js | 2 + client/vitest.config.ts | 2 + package-lock.json | 7 +++ 9 files changed, 135 insertions(+) create mode 100644 client/rtlTextAlias.js create mode 100644 client/src/components/Map/rtlText.test.ts create mode 100644 client/src/components/Map/rtlText.ts diff --git a/client/package.json b/client/package.json index 37c54687d..a61e87958 100644 --- a/client/package.json +++ b/client/package.json @@ -37,6 +37,7 @@ "@fontsource/museomoderno": "^5.3.0", "@fontsource/playfair-display": "^5.3.0", "@fontsource/poppins": "^5.2.7", + "@mapbox/mapbox-gl-rtl-text": "^0.4.0", "@maplibre/maplibre-gl-leaflet": "0.1.4", "@simplewebauthn/browser": "^13.1.2", "@trek/shared": "*", diff --git a/client/rtlTextAlias.js b/client/rtlTextAlias.js new file mode 100644 index 000000000..3bfaa367a --- /dev/null +++ b/client/rtlTextAlias.js @@ -0,0 +1,25 @@ +import { createRequire } from 'node:module'; +import path from 'node:path'; + +/** + * setRTLTextPlugin() takes the URL of a classic script that the GL worker pulls in + * with importScripts. mapbox-gl-rtl-text ships exactly that as + * dist/mapbox-gl-rtl-text.js, but its "exports" field publishes only the ESM/WASM + * entry, so the resolver refuses the subpath. Resolving the entry it does publish + * and stepping across to dist keeps this working wherever npm hoists the package. + * + * Shared by vite.config.js and vitest.config.ts: the build needs it to emit the + * asset, and the test run needs it because a lazily imported engine module reaches + * the same specifier. + * + * The pattern is a regex, not a string key: the import carries a `?url` suffix, + * which a string alias will not match. It is left unanchored on purpose so the + * query survives the rewrite and Vite still emits an asset URL. + */ +export const rtlTextAlias = { + find: /^@mapbox\/mapbox-gl-rtl-text\/dist\/mapbox-gl-rtl-text\.js/, + replacement: path.join( + path.dirname(createRequire(import.meta.url).resolve('@mapbox/mapbox-gl-rtl-text')), + '../dist/mapbox-gl-rtl-text.js', + ), +}; diff --git a/client/src/components/Map/engines/mapbox.ts b/client/src/components/Map/engines/mapbox.ts index f1e114494..c8f74c476 100644 --- a/client/src/components/Map/engines/mapbox.ts +++ b/client/src/components/Map/engines/mapbox.ts @@ -1,5 +1,7 @@ import mapboxgl from 'mapbox-gl' import 'mapbox-gl/dist/mapbox-gl.css' +import rtlTextPluginUrl from '@mapbox/mapbox-gl-rtl-text/dist/mapbox-gl-rtl-text.js?url' +import { registerRtlTextPlugin } from '../rtlText' /** * The only module in the client that pulls mapbox-gl in at runtime. @@ -8,4 +10,8 @@ import 'mapbox-gl/dist/mapbox-gl.css' * a static import from anywhere in the tree drags 1.8 MB into the parent chunk and * quietly undoes the split. `npm run check:gl-split` fails on that. */ +// Arabic/Hebrew/Persian labels come out unjoined and reversed without this. +// Mapbox's signature is (url, callback, deferred) — see rtlText.ts. +registerRtlTextPlugin(() => mapboxgl.setRTLTextPlugin(rtlTextPluginUrl, null, true)) + export default mapboxgl diff --git a/client/src/components/Map/engines/maplibre.ts b/client/src/components/Map/engines/maplibre.ts index 662c13994..d67716a70 100644 --- a/client/src/components/Map/engines/maplibre.ts +++ b/client/src/components/Map/engines/maplibre.ts @@ -1,5 +1,7 @@ import maplibregl from 'maplibre-gl' import 'maplibre-gl/dist/maplibre-gl.css' +import rtlTextPluginUrl from '@mapbox/mapbox-gl-rtl-text/dist/mapbox-gl-rtl-text.js?url' +import { registerRtlTextPlugin } from '../rtlText' /** * The only module in the client that pulls maplibre-gl in at runtime. Same rule as @@ -10,4 +12,8 @@ import 'maplibre-gl/dist/maplibre-gl.css' * stay engine-agnostic. Where the two genuinely differ, the caller already guards * on the provider (projection, accessToken, aroundCenter). */ +// Arabic/Hebrew/Persian labels come out unjoined and reversed without this. +// MapLibre's signature is (url, lazy) and it resolves a promise — see rtlText.ts. +registerRtlTextPlugin(() => maplibregl.setRTLTextPlugin(rtlTextPluginUrl, true)) + export default maplibregl diff --git a/client/src/components/Map/rtlText.test.ts b/client/src/components/Map/rtlText.test.ts new file mode 100644 index 000000000..2715ee348 --- /dev/null +++ b/client/src/components/Map/rtlText.test.ts @@ -0,0 +1,38 @@ +import { describe, expect, it, vi } from 'vitest' +import { registerRtlTextPlugin } from './rtlText' + +describe('registerRtlTextPlugin', () => { + it('runs the engine-supplied registration', () => { + const register = vi.fn() + + expect(registerRtlTextPlugin(register)).toBe(true) + expect(register).toHaveBeenCalledTimes(1) + }) + + // Registering twice is how the engines report it: Mapbox throws. + it('never lets a refused registration break map creation', () => { + const register = vi.fn(() => { + throw new Error('setRTLTextPlugin cannot be called multiple times.') + }) + + expect(() => registerRtlTextPlugin(register)).not.toThrow() + expect(registerRtlTextPlugin(register)).toBe(false) + }) + + // MapLibre reports the same by rejecting, and an unhandled rejection would + // reach the console for something the user cannot act on. + it('swallows a rejected registration promise', async () => { + const onUnhandled = vi.fn() + process.on('unhandledRejection', onUnhandled) + + expect(registerRtlTextPlugin(() => Promise.reject(new Error('404')))).toBe(true) + await new Promise(resolve => setTimeout(resolve, 0)) + process.off('unhandledRejection', onUnhandled) + + expect(onUnhandled).not.toHaveBeenCalled() + }) + + it('leaves a resolved promise alone', async () => { + expect(registerRtlTextPlugin(() => Promise.resolve())).toBe(true) + }) +}) diff --git a/client/src/components/Map/rtlText.ts b/client/src/components/Map/rtlText.ts new file mode 100644 index 000000000..c260772ec --- /dev/null +++ b/client/src/components/Map/rtlText.ts @@ -0,0 +1,48 @@ +/** + * Arabic, Hebrew, Persian and Urdu labels need two things a GL engine does not do + * on its own: the bidirectional algorithm, and Arabic contextual shaping (a letter + * is drawn differently depending on its neighbours). Without them the engine lays + * the codepoints out one by one, left to right, in isolated forms — so القاهرة + * comes out as ة ر ه ا ق ل ا, unjoined and reversed. Raster basemaps never showed + * this because their labels are drawn server-side, which is why it only surfaced + * once the default basemap became a vector style. + * + * Both engines expose the fix as an optional plugin and neither loads it by + * default. Registering is global to the engine, so it happens once per engine + * module rather than at each map's construction site. + * + * The call itself stays with the caller: this is one of the places the two engines + * genuinely differ. MapLibre 5 takes `(url, lazy)` and returns a promise, Mapbox + * GL 3 takes `(url, callback, deferred)` and returns nothing — so each engine + * module passes its own invocation rather than a shared wrapper guessing an arity. + * + * Deliberately no getRTLTextPluginStatus() pre-check: reading it makes MapLibre + * build its worker pool, which is an eager side effect for a module that is + * imported for its default export, and throws outright under jsdom. Registering + * twice is reported — by a throw or a rejected promise depending on the engine — + * and both are handled below, so the probe bought nothing. + */ + +/** + * Runs a GL engine's RTL-plugin registration, swallowing the ways it can fail. + * + * Callers register lazily, so the payload is only fetched once a map actually + * paints RTL text and a user who never leaves Latin script downloads nothing. + * + * @returns whether the registration call completed without throwing. + */ +export function registerRtlTextPlugin(register: () => void | Promise): boolean { + try { + const pending = register() + // MapLibre reports a failed or duplicate registration by rejecting; Mapbox + // routes the same through its callback. Neither is worth surfacing: a map + // with unshaped labels still renders, and an unhandled rejection would not + // give the user anything to act on. + if (pending instanceof Promise) pending.catch(() => {}) + return true + } catch { + // Already registered, or the engine refused outright. Registering the plugin + // must never be able to break map creation. + return false + } +} diff --git a/client/vite.config.js b/client/vite.config.js index 985d070d2..30e28be1c 100644 --- a/client/vite.config.js +++ b/client/vite.config.js @@ -2,6 +2,7 @@ import react from '@vitejs/plugin-react'; import { defineConfig } from 'vite'; import { VitePWA } from 'vite-plugin-pwa'; import { visualizer } from 'rollup-plugin-visualizer'; +import { rtlTextAlias } from './rtlTextAlias.js'; // `npm run build:analyze` writes dist/stats.html — a treemap of what actually ended // up in each chunk. The plain build only reports chunk sizes, which tells you a chunk @@ -227,6 +228,7 @@ export default defineConfig(({ mode }) => ({ }, }), ].filter(Boolean), + resolve: { alias: [rtlTextAlias] }, build: { // Pin the output level instead of inheriting whatever the current Vite default // is, so a toolchain bump can't silently change which browsers still work. diff --git a/client/vitest.config.ts b/client/vitest.config.ts index 8c9dc4ae6..8c135d1f3 100644 --- a/client/vitest.config.ts +++ b/client/vitest.config.ts @@ -1,9 +1,11 @@ import { defineConfig } from 'vitest/config'; import react from '@vitejs/plugin-react'; +import { rtlTextAlias } from './rtlTextAlias.js'; export default defineConfig({ plugins: [react()], test: { + alias: [rtlTextAlias], root: '.', globals: true, environment: './tests/environment/jsdom-native-abort.ts', diff --git a/package-lock.json b/package-lock.json index 9cc831179..9bc7a11c2 100644 --- a/package-lock.json +++ b/package-lock.json @@ -27,6 +27,7 @@ "name": "@trek/client", "version": "4.1.1", "dependencies": { + "@mapbox/mapbox-gl-rtl-text": "^0.4.0", "@fontsource/bebas-neue": "^5.3.0", "@fontsource/eb-garamond": "^5.3.0", "@fontsource/geist-sans": "^5.2.5", @@ -4699,6 +4700,12 @@ "node": ">= 22" } }, + "node_modules/@mapbox/mapbox-gl-rtl-text": { + "version": "0.4.0", + "resolved": "https://registry.npmjs.org/@mapbox/mapbox-gl-rtl-text/-/mapbox-gl-rtl-text-0.4.0.tgz", + "integrity": "sha512-WNSgEFxXd2a15Eh2GwZNWBiV7Z8leq8JEGcoHpfrgaBDmFQMW4a1MO5Ol9BNv6APqTbHZ8uq7L8H5q2KCVt1kg==", + "license": "BSD-2-Clause" + }, "node_modules/@mapbox/point-geometry": { "version": "1.1.0", "resolved": "https://registry.npmjs.org/@mapbox/point-geometry/-/point-geometry-1.1.0.tgz",