From 5c9d2813322810e847473b0c852af4ff46f139ae Mon Sep 17 00:00:00 2001 From: Kai-W Date: Mon, 25 Mar 2024 16:48:08 +0100 Subject: [PATCH 1/2] 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 --- CHANGELOG.md | 1 + src/style/load_sprite.test.ts | 91 ++++++++++++++++++++++++++++------- src/style/load_sprite.ts | 20 +++++--- src/style/style.test.ts | 8 ++- src/util/request_manager.ts | 3 +- 5 files changed, 92 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9db7e1c7e1..d46e77172c 100644 --- 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 index 4dc379a458..b2ff73fda1 100644 --- 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('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/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 index 12b270c342..391317442b 100644 --- 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>} = {}; for (const {id, url} of spriteArray) { - const jsonRequestParameters = requestManager.transformRequest(normalizeSpriteURL(url, format, '.json'), ResourceType.SpriteJSON); - jsonsMap[id] = getJSON(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({ + ...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 index 6060cfc086..81be874364 100644 --- 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(done => { diff --git a/src/util/request_manager.ts b/src/util/request_manager.ts index bb98b284ea..f679dd0f85 100644 --- 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', From 789f15801b53d7ae65991d8cb4a2c1553b50a676 Mon Sep 17 00:00:00 2001 From: Harel M Date: Tue, 22 Oct 2024 21:10:55 +0300 Subject: [PATCH 2/2] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d46e77172c..f4ca8d322e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +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)) +- ⚠️ 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