fix: order of normalizeSpriteURL and transformRequest in load sprites (…

…maplibre#3898) * changed order of normalize and transform in load sprites transformRequest is now called once with ResourceType.Sprite and not seperatly for SpiteJSON and SpriteImage. URL validation and appending of json/png is done afterwards * Update CHANGELOG.md --------- Co-authored-by: Harel M <[email protected]>
jcary741 · Oct 22, 2024 · 54cf54e · 54cf54e
1 parent f24dfe6
commit 54cf54e
Show file tree

Hide file tree

Showing 5 changed files with 92 additions and 31 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,7 @@
 - _...Add new stuff here..._
 
 ### 🐞 Bug fixes
+- ⚠️ Fix order of normalizeSpriteURL and transformRequest in loadSprite ([#3897](https://github.com/maplibre/maplibre-gl-js/issues/3897))
 - _...Add new stuff here..._
 
 ## v5.0.0-pre.3

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/[email protected]?fresh=true');
     });
 
-    test('test relative URL', () => {
+    test('No Path', () => {
         expect(
-            normalizeSpriteURL('/bar?fresh=true', '@2x', '.png')
-        ).toBe('/bar@2x.png?fresh=true');
+            normalizeSpriteURL('http://www.foo.com?fresh=true', '@2x', '.json')
+        ).toBe('http://www.foo.com/@2x.json?fresh=true');
     });
 });
 
@@ -66,9 +66,70 @@ describe('loadSprite', () => {
 
         const result = await promise;
 
-        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(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(Object.keys(result)).toHaveLength(1);
         expect(Object.keys(result)[0]).toBe('default');
@@ -98,9 +159,8 @@ describe('loadSprite', () => {
 
         const result = await promise;
 
-        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(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');
@@ -131,11 +191,9 @@ describe('loadSprite', () => {
         server.respond();
 
         const result = await promise;
-        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(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(Object.keys(result)).toHaveLength(2);
         expect(Object.keys(result)[0]).toBe('sprite1');
@@ -209,9 +267,8 @@ describe('loadSprite', () => {
         server.respond();
 
         const result = await promise;
-        expect(transform).toHaveBeenCalledTimes(2);
-        expect(transform).toHaveBeenNthCalledWith(1, 'http://localhost:9966/test/unit/assets/[email protected]', 'SpriteJSON');
-        expect(transform).toHaveBeenNthCalledWith(2, 'http://localhost:9966/test/unit/assets/[email protected]', 'SpriteImage');
+        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');

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

diff --git a/src/style/style.test.ts b/src/style/style.test.ts
@@ -340,11 +340,9 @@ describe('Style#loadJSON', () => {
 
         await style.once('style.load');
 
-        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');
+        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');
     });
 
     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,8 +7,7 @@ export const enum ResourceType {
     Glyphs = 'Glyphs',
     Image = 'Image',
     Source = 'Source',
-    SpriteImage = 'SpriteImage',
-    SpriteJSON = 'SpriteJSON',
+    Sprite = 'Sprite',
     Style = 'Style',
     Tile = 'Tile',
     Unknown = 'Unknown',