changed order of normalize and transform in load sprites

maplibre · Mar 31, 2024 · aa5f46d · aa5f46d
1 parent 5457628
commit aa5f46d
Show file tree

Hide file tree

Showing 5 changed files with 55 additions and 26 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,7 +4,7 @@
 - Added const enum for actor messages to improve readability and maintainability. In tsconfig.json, `isolatedModules` flag is set to false in favor of generated JS size. ([#3879](https://github.com/maplibre/maplibre-gl-js/issues/3879))
 
 ### 🐞 Bug fixes
-- Fix normalizeSpriteURL before transformRequest throwing an Error with reelative URLs ([#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))
 - _...Add new stuff here..._
 
 ## 4.1.2

diff --git a/src/style/load_sprite.test.ts b/src/style/load_sprite.test.ts
@@ -66,9 +66,39 @@ 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 invalid 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');
@@ -98,9 +128,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 +160,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 +236,8 @@ describe('loadSprite', () => {
         server.respond();
 
         const result = await promise;
-        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(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
@@ -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
@@ -338,11 +338,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', 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',