Revert "fix: order of normalizeSpriteURL and transformRequest in load…

… sprites (maplibre#3898)" This reverts commit 54cf54e
jcary741 · Nov 2, 2024 · ee56a63 · ee56a63
1 parent ef6e9ee
commit ee56a63
Show file tree

Hide file tree

Showing 5 changed files with 32 additions and 92 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -21,7 +21,7 @@
 
 ### 🐞 Bug fixes
 
-- ⚠️ Fix order of normalizeSpriteURL and transformRequest in loadSprite ([#3897](https://github.com/maplibre/maplibre-gl-js/issues/3897))
+- ⚠️ ~Fix order of normalizeSpriteURL and transformRequest in loadSprite ([#3897](https://github.com/maplibre/maplibre-gl-js/issues/3897))~
 - ⚠️ Remove unminified prod build ([#4906](https://github.com/maplibre/maplibre-gl-js/pull/4906))
 - Fix issue where raster tile source won't fetch updates following request error ([#4890](https://github.com/maplibre/maplibre-gl-js/pull/4890))
 - Fix 3D models in custom layers not being properly occluded by the globe ([#4817](https://github.com/maplibre/maplibre-gl-js/issues/4817))

diff --git a/src/style/load_sprite.test.ts b/src/style/load_sprite.test.ts
@@ -26,10 +26,10 @@ describe('normalizeSpriteURL', () => {
         ).toBe('http://www.foo.com/bar@2x.png?fresh=true');
     });
 
-    test('No Path', () => {
+    test('test relative URL', () => {
         expect(
-            normalizeSpriteURL('http://www.foo.com?fresh=true', '@2x', '.json')
-        ).toBe('http://www.foo.com/@2x.json?fresh=true');
+            normalizeSpriteURL('/bar?fresh=true', '@2x', '.png')
+        ).toBe('/bar@2x.png?fresh=true');
     });
 });
 
@@ -66,70 +66,9 @@ describe('loadSprite', () => {
 
         const result = await promise;
 
-        expect(transform).toHaveBeenCalledTimes(1);
-        expect(transform).toHaveBeenNthCalledWith(1, 'http://localhost:9966/test/unit/assets/sprite1', 'Sprite');
-
-        expect(Object.keys(result)).toHaveLength(1);
-        expect(Object.keys(result)[0]).toBe('default');
-
-        Object.values(result['default']).forEach(styleImage => {
-            expect(styleImage.spriteData).toBeTruthy();
-            expect(styleImage.spriteData.context).toBeInstanceOf(CanvasRenderingContext2D);
-        });
-
-        expect(server.requests[0].url).toBe('http://localhost:9966/test/unit/assets/sprite1.json');
-        expect(server.requests[1].url).toBe('http://localhost:9966/test/unit/assets/sprite1.png');
-    });
-
-    test('transform of relative url', async () => {
-        const transform = jest.fn().mockImplementation((url, type) => {
-            return {url: `http://localhost:9966${url}`, type};
-        });
-
-        const manager = new RequestManager(transform);
-
-        server.respondWith('GET', 'http://localhost:9966/test/unit/assets/sprite1.json', fs.readFileSync(path.join(__dirname, '../../test/unit/assets/sprite1.json')).toString());
-        server.respondWith('GET', 'http://localhost:9966/test/unit/assets/sprite1.png', bufferToArrayBuffer(fs.readFileSync(path.join(__dirname, '../../test/unit/assets/sprite1.png'))));
-
-        const promise = loadSprite('/test/unit/assets/sprite1', manager, 1, new AbortController());
-
-        server.respond();
-
-        const result = await promise;
-
-        expect(transform).toHaveBeenCalledTimes(1);
-        expect(transform).toHaveBeenNthCalledWith(1, '/test/unit/assets/sprite1', 'Sprite');
-
-        expect(Object.keys(result)).toHaveLength(1);
-        expect(Object.keys(result)[0]).toBe('default');
-
-        Object.values(result['default']).forEach(styleImage => {
-            expect(styleImage.spriteData).toBeTruthy();
-            expect(styleImage.spriteData.context).toBeInstanceOf(CanvasRenderingContext2D);
-        });
-
-        expect(server.requests[0].url).toBe('http://localhost:9966/test/unit/assets/sprite1.json');
-        expect(server.requests[1].url).toBe('http://localhost:9966/test/unit/assets/sprite1.png');
-    });
-
-    test('transform of random Sprite String', async () => {
-        const transform = jest.fn().mockImplementation((url, type) => {
-            return {url: 'http://localhost:9966/test/unit/assets/sprite1', type};
-        });
-
-        const manager = new RequestManager(transform);
-
-        server.respondWith('GET', 'http://localhost:9966/test/unit/assets/sprite1.json', fs.readFileSync(path.join(__dirname, '../../test/unit/assets/sprite1.json')).toString());
-        server.respondWith('GET', 'http://localhost:9966/test/unit/assets/sprite1.png', bufferToArrayBuffer(fs.readFileSync(path.join(__dirname, '../../test/unit/assets/sprite1.png'))));
-
-        const promise = loadSprite('foobar', manager, 1, new AbortController());
-
-        server.respond();
-
-        const result = await promise;
-
-        expect(transform).toHaveBeenCalledTimes(1);
-        expect(transform).toHaveBeenNthCalledWith(1, 'foobar', 'Sprite');
+        expect(transform).toHaveBeenCalledTimes(2);
+        expect(transform).toHaveBeenNthCalledWith(1, 'http://localhost:9966/test/unit/assets/sprite1.json', 'SpriteJSON');
+        expect(transform).toHaveBeenNthCalledWith(2, 'http://localhost:9966/test/unit/assets/sprite1.png', 'SpriteImage');
 
         expect(Object.keys(result)).toHaveLength(1);
         expect(Object.keys(result)[0]).toBe('default');
@@ -159,8 +98,9 @@ describe('loadSprite', () => {
 
         const result = await promise;
 
-        expect(transform).toHaveBeenCalledTimes(1);
-        expect(transform).toHaveBeenNthCalledWith(1, '/test/unit/assets/sprite1', 'Sprite');
+        expect(transform).toHaveBeenCalledTimes(2);
+        expect(transform).toHaveBeenNthCalledWith(1, '/test/unit/assets/sprite1.json', 'SpriteJSON');
+        expect(transform).toHaveBeenNthCalledWith(2, '/test/unit/assets/sprite1.png', 'SpriteImage');
 
         expect(Object.keys(result)).toHaveLength(1);
         expect(Object.keys(result)[0]).toBe('default');
@@ -191,9 +131,11 @@ describe('loadSprite', () => {
         server.respond();
 
         const result = await promise;
-        expect(transform).toHaveBeenCalledTimes(2);
-        expect(transform).toHaveBeenNthCalledWith(1, 'http://localhost:9966/test/unit/assets/sprite1', 'Sprite');
-        expect(transform).toHaveBeenNthCalledWith(2, 'http://localhost:9966/test/unit/assets/sprite2', 'Sprite');
+        expect(transform).toHaveBeenCalledTimes(4);
+        expect(transform).toHaveBeenNthCalledWith(1, 'http://localhost:9966/test/unit/assets/sprite1.json', 'SpriteJSON');
+        expect(transform).toHaveBeenNthCalledWith(2, 'http://localhost:9966/test/unit/assets/sprite1.png', 'SpriteImage');
+        expect(transform).toHaveBeenNthCalledWith(3, 'http://localhost:9966/test/unit/assets/sprite2.json', 'SpriteJSON');
+        expect(transform).toHaveBeenNthCalledWith(4, 'http://localhost:9966/test/unit/assets/sprite2.png', 'SpriteImage');
 
         expect(Object.keys(result)).toHaveLength(2);
         expect(Object.keys(result)[0]).toBe('sprite1');
@@ -267,8 +209,9 @@ describe('loadSprite', () => {
         server.respond();
 
         const result = await promise;
-        expect(transform).toHaveBeenCalledTimes(1);
-        expect(transform).toHaveBeenNthCalledWith(1, 'http://localhost:9966/test/unit/assets/sprite1', 'Sprite');
+        expect(transform).toHaveBeenCalledTimes(2);
+        expect(transform).toHaveBeenNthCalledWith(1, 'http://localhost:9966/test/unit/assets/sprite1@2x.json', 'SpriteJSON');
+        expect(transform).toHaveBeenNthCalledWith(2, 'http://localhost:9966/test/unit/assets/sprite1@2x.png', 'SpriteImage');
 
         expect(Object.keys(result)).toHaveLength(1);
         expect(Object.keys(result)[0]).toBe('default');

diff --git a/src/style/load_sprite.ts b/src/style/load_sprite.ts
@@ -16,9 +16,9 @@ export type LoadSpriteResult = {
 }
 
 export function normalizeSpriteURL(url: string, format: string, extension: string): string {
-    const parsed = new URL(url);
-    parsed.pathname += `${format}${extension}`;
-    return parsed.toString();
+    const split = url.split('?');
+    split[0] += `${format}${extension}`;
+    return split.join('?');
 }
 
 export async function loadSprite(
@@ -34,17 +34,11 @@ export async function loadSprite(
     const imagesMap: {[id: string]: Promise<GetResourceResponse<HTMLImageElement | ImageBitmap>>} = {};
 
     for (const {id, url} of spriteArray) {
-        const requestParameters = requestManager.transformRequest(url, ResourceType.Sprite);
+        const jsonRequestParameters = requestManager.transformRequest(normalizeSpriteURL(url, format, '.json'), ResourceType.SpriteJSON);
+        jsonsMap[id] = getJSON<SpriteJSON>(jsonRequestParameters, abortController);
 
-        jsonsMap[id] = getJSON<SpriteJSON>({
-            ...requestParameters,
-            url: normalizeSpriteURL(requestParameters.url, format, '.json')
-        }, abortController);
-
-        imagesMap[id] = ImageRequest.getImage({
-            ...requestParameters,
-            url: normalizeSpriteURL(requestParameters.url, format, '.png')
-        }, abortController);
+        const imageRequestParameters = requestManager.transformRequest(normalizeSpriteURL(url, format, '.png'), ResourceType.SpriteImage);
+        imagesMap[id] = ImageRequest.getImage(imageRequestParameters, abortController);
     }
 
     await Promise.all([...Object.values(jsonsMap), ...Object.values(imagesMap)]);

diff --git a/src/style/style.test.ts b/src/style/style.test.ts
@@ -340,9 +340,11 @@ describe('Style#loadJSON', () => {
 
         await style.once('style.load');
 
-        expect(transformSpy).toHaveBeenCalledTimes(1);
-        expect(transformSpy.mock.calls[0][0]).toBe('http://example.com/sprites/bright-v8');
-        expect(transformSpy.mock.calls[0][1]).toBe('Sprite');
+        expect(transformSpy).toHaveBeenCalledTimes(2);
+        expect(transformSpy.mock.calls[0][0]).toBe('http://example.com/sprites/bright-v8.json');
+        expect(transformSpy.mock.calls[0][1]).toBe('SpriteJSON');
+        expect(transformSpy.mock.calls[1][0]).toBe('http://example.com/sprites/bright-v8.png');
+        expect(transformSpy.mock.calls[1][1]).toBe('SpriteImage');
     });
 
     test('emits an error on non-existant vector source layer', () => new Promise<void>(done => {

diff --git a/src/util/request_manager.ts b/src/util/request_manager.ts
@@ -7,7 +7,8 @@ export const enum ResourceType {
     Glyphs = 'Glyphs',
     Image = 'Image',
     Source = 'Source',
-    Sprite = 'Sprite',
+    SpriteImage = 'SpriteImage',
+    SpriteJSON = 'SpriteJSON',
     Style = 'Style',
     Tile = 'Tile',
     Unknown = 'Unknown',