From 51c5b29fcdce68d377fb3fbf15fc74f3fd697ff3 Mon Sep 17 00:00:00 2001 From: Vjacheslav Trushkin Date: Sun, 3 Nov 2024 09:35:48 +0200 Subject: [PATCH] chore(core): refactor loader to check icon names only in API calls --- packages/core/src/api/icons.ts | 139 +++-- packages/core/src/api/modules/fetch.ts | 1 + packages/core/tests/api/loading-test.ts | 6 +- packages/core/tests/api/mock-test.ts | 661 +++++++++++++---------- packages/utils/tests/parse-icons-test.ts | 43 ++ 5 files changed, 518 insertions(+), 332 deletions(-) diff --git a/packages/core/src/api/icons.ts b/packages/core/src/api/icons.ts index fec328a..058894d 100644 --- a/packages/core/src/api/icons.ts +++ b/packages/core/src/api/icons.ts @@ -1,5 +1,9 @@ import type { IconifyIcon, IconifyJSON } from '@iconify/types'; -import { IconifyIconName, stringToIcon } from '@iconify/utils/lib/icon/name'; +import { + IconifyIconName, + matchIconName, + stringToIcon, +} from '@iconify/utils/lib/icon/name'; import type { SortedIcons } from '../icon/sort'; import { sortIcons } from '../icon/sort'; import { storeCallback, updateCallbacks } from './callbacks'; @@ -61,6 +65,71 @@ function loadedNewIcons(storage: IconStorageWithAPI): void { } } +interface CheckIconNames { + valid: string[]; + invalid: string[]; +} +/** + * Check icon names for API + */ +function checkIconNamesForAPI(icons: string[]): CheckIconNames { + const valid: string[] = []; + const invalid: string[] = []; + + icons.forEach((name) => { + (name.match(matchIconName) ? valid : invalid).push(name); + }); + return { + valid, + invalid, + }; +} + +/** + * Parse loader response + */ +function parseLoaderResponse( + storage: IconStorageWithAPI, + icons: string[], + data: unknown +) { + const fail = () => { + icons.forEach((name) => { + storage.missing.add(name); + }); + }; + + // Check for error + if (typeof data !== 'object' || !data) { + fail(); + } else { + // Add icons to storage + try { + const parsed = addIconSet(storage, data as IconifyJSON); + if (!parsed.length) { + fail(); + return; + } + + // Remove added icons from pending list + const pending = storage.pendingIcons; + if (pending) { + parsed.forEach((name) => { + pending.delete(name); + }); + } + + // Cache API response + storeInBrowserStorage(storage, data as IconifyJSON); + } catch (err) { + console.error(err); + } + } + + // Trigger update on next tick + loadedNewIcons(storage); +} + /** * Load icons */ @@ -80,54 +149,40 @@ function loadNewIcons(storage: IconStorageWithAPI, icons: string[]): void { const { provider, prefix } = storage; // Get icons and delete queue + // Icons should not be undefined, but just in case assume it can be const icons = storage.iconsToLoad; delete storage.iconsToLoad; + // TODO: check for custom loader + + // Using API loader + // Validate icon names for API + const { valid, invalid } = checkIconNamesForAPI(icons || []); + + if (invalid.length) { + // Invalid icons + parseLoaderResponse(storage, invalid, null); + } + if (!valid.length) { + // No valid icons to load + return; + } + // Get API module - let api: ReturnType; - if (!icons || !(api = getAPIModule(provider))) { - // No icons or no way to load icons! + const api = prefix.match(matchIconName) + ? getAPIModule(provider) + : null; + if (!api) { + // API module not found + parseLoaderResponse(storage, valid, null); return; } // Prepare parameters and run queries - const params = api.prepare(provider, prefix, icons); + const params = api.prepare(provider, prefix, valid); params.forEach((item) => { sendAPIQuery(provider, item, (data) => { - // Check for error - if (typeof data !== 'object') { - // Not found: mark as missing - item.icons.forEach((name) => { - storage.missing.add(name); - }); - } else { - // Add icons to storage - try { - const parsed = addIconSet( - storage, - data as IconifyJSON - ); - if (!parsed.length) { - return; - } - - // Remove added icons from pending list - const pending = storage.pendingIcons; - if (pending) { - parsed.forEach((name) => { - pending.delete(name); - }); - } - - // Cache API response - storeInBrowserStorage(storage, data as IconifyJSON); - } catch (err) { - console.error(err); - } - } - - // Trigger update on next tick - loadedNewIcons(storage); + parseLoaderResponse(storage, item.icons, data); }); }); }); @@ -232,9 +287,9 @@ export const loadIcons: IconifyLoadIcons = ( // Load icons on next tick to make sure result is not returned before callback is stored and // to consolidate multiple synchronous loadIcons() calls into one asynchronous API call sources.forEach((storage) => { - const { provider, prefix } = storage; - if (newIcons[provider][prefix].length) { - loadNewIcons(storage, newIcons[provider][prefix]); + const list = newIcons[storage.provider][storage.prefix]; + if (list.length) { + loadNewIcons(storage, list); } }); diff --git a/packages/core/src/api/modules/fetch.ts b/packages/core/src/api/modules/fetch.ts index e1f2234..949f322 100644 --- a/packages/core/src/api/modules/fetch.ts +++ b/packages/core/src/api/modules/fetch.ts @@ -21,6 +21,7 @@ const detectFetch = (): FetchType | undefined => { if (typeof callback === 'function') { return callback; } + // eslint-disable-next-line @typescript-eslint/no-unused-vars } catch (err) { // } diff --git a/packages/core/tests/api/loading-test.ts b/packages/core/tests/api/loading-test.ts index 08bfa78..8c5d581 100644 --- a/packages/core/tests/api/loading-test.ts +++ b/packages/core/tests/api/loading-test.ts @@ -311,9 +311,9 @@ describe('Testing API loadIcons', () => { expect(loadedIcon).toBe(false); // Test isPending - expect(isPending({ provider, prefix, name: 'BadIconName' })).toBe( - false - ); + // After change to naming convention, icon name is valid and should be pending + // Filtering invalid names is done in loader, not in API module + expect(isPending({ provider, prefix, name: 'BadIconName' })).toBe(true); }); it('Loading one icon twice with Promise', () => { diff --git a/packages/core/tests/api/mock-test.ts b/packages/core/tests/api/mock-test.ts index bda77ab..48cf3a8 100644 --- a/packages/core/tests/api/mock-test.ts +++ b/packages/core/tests/api/mock-test.ts @@ -29,102 +29,110 @@ describe('Testing mock API module', () => { // Tests it('404 response', () => { - return new Promise((fulfill) => { + return new Promise((resolve, reject) => { const prefix = nextPrefix(); - - mockAPIData({ - type: 'icons', - provider, - prefix, - icons: ['test1', 'test2'], - response: 404, - }); - let isSync = true; - loadIcons( - [ - { - provider, - prefix, - name: 'test1', - }, - ], - (loaded, missing, pending) => { - expect(isSync).toBe(false); - expect(loaded).toEqual([]); - expect(pending).toEqual([]); - expect(missing).toEqual([ + try { + mockAPIData({ + type: 'icons', + provider, + prefix, + icons: ['test1', 'test2'], + response: 404, + }); + + loadIcons( + [ { provider, prefix, name: 'test1', }, - ]); - fulfill(true); - } - ); + ], + (loaded, missing, pending) => { + try { + expect(isSync).toBe(false); + expect(loaded).toEqual([]); + expect(pending).toEqual([]); + expect(missing).toEqual([ + { + provider, + prefix, + name: 'test1', + }, + ]); + } catch (error) { + reject(error); + return; + } + resolve(true); + } + ); + } catch (error) { + reject(error); + } isSync = false; }); }); it('Load few icons', () => { - return new Promise((fulfill) => { + return new Promise((resolve, reject) => { const prefix = nextPrefix(); - - mockAPIData({ - type: 'icons', - provider, - prefix, - response: { - prefix, - icons: { - test10: { - body: '', - }, - test11: { - body: '', - }, - }, - }, - }); - mockAPIData({ - type: 'icons', - provider, - prefix, - response: { - prefix, - icons: { - test20: { - body: '', - }, - test21: { - body: '', - }, - }, - }, - }); - let isSync = true; - loadIcons( - [ - { - provider, + try { + mockAPIData({ + type: 'icons', + provider, + prefix, + response: { prefix, - name: 'test10', + icons: { + test10: { + body: '', + }, + test11: { + body: '', + }, + }, }, - { - provider, + }); + mockAPIData({ + type: 'icons', + provider, + prefix, + response: { prefix, - name: 'test20', + icons: { + test20: { + body: '', + }, + test21: { + body: '', + }, + }, }, - ], - (loaded, missing, pending) => { - expect(isSync).toBe(false); - // All icons should have been loaded because API waits one tick before sending response, during which both queries are processed - expect(loaded).toEqual([ + }); + // Data with invalid name: should not be requested from API because name is invalid + // Split from main data because otherwise it would load when other icons are requested + mockAPIData({ + type: 'icons', + provider, + prefix, + response: { + prefix, + icons: { + BadName: { + body: '', + }, + }, + }, + }); + + loadIcons( + [ { provider, prefix, @@ -135,283 +143,362 @@ describe('Testing mock API module', () => { prefix, name: 'test20', }, - ]); - expect(pending).toEqual([]); - expect(missing).toEqual([]); - fulfill(true); - } - ); + { + provider, + prefix, + name: 'BadName', + }, + ], + (loaded, missing, pending) => { + try { + if (pending.length) { + // Not ready to test yet + return; + } + + expect(isSync).toBe(false); + // All icons should have been loaded because API waits one tick before sending response, during which both queries are processed + expect(loaded).toEqual([ + { + provider, + prefix, + name: 'test10', + }, + { + provider, + prefix, + name: 'test20', + }, + ]); + expect(pending).toEqual([]); + expect(missing).toEqual([ + { + provider, + prefix, + name: 'BadName', + }, + ]); + } catch (error) { + reject(error); + return; + } + resolve(true); + } + ); + } catch (error) { + reject(error); + } isSync = false; }); }); it('Load in batches and testing delay', () => { - return new Promise((fulfill, reject) => { + return new Promise((resolve, reject) => { const prefix = nextPrefix(); let next: IconifyMockAPIDelayDoneCallback | undefined; - - mockAPIData({ - type: 'icons', - provider, - prefix, - response: { - prefix, - icons: { - test10: { - body: '', - }, - test11: { - body: '', - }, - }, - }, - }); - mockAPIData({ - type: 'icons', - provider, - prefix, - response: { - prefix, - icons: { - test20: { - body: '', - }, - test21: { - body: '', - }, - }, - }, - delay: (callback) => { - next = callback; - }, - }); - let callbackCounter = 0; - loadIcons( - [ - { - provider, + try { + mockAPIData({ + type: 'icons', + provider, + prefix, + response: { prefix, - name: 'test10', + icons: { + test10: { + body: '', + }, + test11: { + body: '', + }, + }, }, - { - provider, + }); + mockAPIData({ + type: 'icons', + provider, + prefix, + response: { prefix, - name: 'test20', + icons: { + test20: { + body: '', + }, + test21: { + body: '', + }, + }, }, - ], - (loaded, missing, pending) => { - callbackCounter++; - switch (callbackCounter) { - case 1: - // First load: only 'test10' - expect(loaded).toEqual([ - { - provider, - prefix, - name: 'test10', - }, - ]); - expect(pending).toEqual([ - { - provider, - prefix, - name: 'test20', - }, - ]); + delay: (callback) => { + next = callback; + }, + }); - // Send second response - expect(typeof next).toBe('function'); - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - next!(); - break; + loadIcons( + [ + { + provider, + prefix, + name: 'test10', + }, + { + provider, + prefix, + name: 'test20', + }, + ], + (loaded, missing, pending) => { + callbackCounter++; + try { + switch (callbackCounter) { + case 1: + // First load: only 'test10' + expect(loaded).toEqual([ + { + provider, + prefix, + name: 'test10', + }, + ]); + expect(pending).toEqual([ + { + provider, + prefix, + name: 'test20', + }, + ]); - case 2: - // All icons should have been loaded - expect(loaded).toEqual([ - { - provider, - prefix, - name: 'test10', - }, - { - provider, - prefix, - name: 'test20', - }, - ]); - expect(missing).toEqual([]); - fulfill(true); - break; + // Send second response + expect(typeof next).toBe('function'); + next!(); + break; - default: - reject( - 'Callback was called more times than expected' - ); + case 2: + // All icons should have been loaded + expect(loaded).toEqual([ + { + provider, + prefix, + name: 'test10', + }, + { + provider, + prefix, + name: 'test20', + }, + ]); + expect(missing).toEqual([]); + resolve(true); + break; + + default: + reject( + 'Callback was called more times than expected' + ); + } + } catch (error) { + reject(error); + } } - } - ); + ); + } catch (error) { + reject(error); + } }); }); // This is useful for testing component where loadIcons() cannot be accessed it('Using timer in callback for second test', () => { - return new Promise((fulfill) => { + return new Promise((resolve, reject) => { const prefix = nextPrefix(); const name = 'test1'; - // Mock data - mockAPIData({ - type: 'icons', - provider, - prefix, - response: { - prefix, - icons: { - [name]: { - body: '', - }, - }, - }, - delay: (next) => { - // Icon should not be loaded yet - const storage = getStorage(provider, prefix); - expect(iconInStorage(storage, name)).toBe(false); - - // Set data - next(); - - // Icon should be loaded now - expect(iconInStorage(storage, name)).toBe(true); - - fulfill(true); - }, - }); - - // Load icons - loadIcons([ - { + try { + // Mock data + mockAPIData({ + type: 'icons', provider, prefix, - name, - }, - ]); + response: { + prefix, + icons: { + [name]: { + body: '', + }, + }, + }, + delay: (next) => { + try { + // Icon should not be loaded yet + const storage = getStorage(provider, prefix); + expect(iconInStorage(storage, name)).toBe(false); + + // Set data + next(); + + // Icon should be loaded now + expect(iconInStorage(storage, name)).toBe(true); + } catch (error) { + reject(error); + return; + } + resolve(true); + }, + }); + + // Load icons + loadIcons([ + { + provider, + prefix, + name, + }, + ]); + } catch (error) { + reject(error); + } }); }); it('Custom query', () => { - return new Promise((fulfill) => { - mockAPIData({ - type: 'custom', - provider, - uri: '/test', - response: { - foo: true, - }, - }); - + return new Promise((resolve, reject) => { let isSync = true; - sendAPIQuery( - provider, - { + try { + mockAPIData({ type: 'custom', provider, uri: '/test', - }, - (data, error) => { - expect(error).toBeUndefined(); - expect(data).toEqual({ + response: { foo: true, - }); - expect(isSync).toBe(false); - fulfill(true); - } - ); + }, + }); + + sendAPIQuery( + provider, + { + type: 'custom', + provider, + uri: '/test', + }, + (data, error) => { + try { + expect(error).toBeUndefined(); + expect(data).toEqual({ + foo: true, + }); + expect(isSync).toBe(false); + } catch (error) { + reject(error); + return; + } + resolve(true); + } + ); + } catch (error) { + reject(error); + } isSync = false; }); }); it('Custom query with host', () => { - return new Promise((fulfill) => { + return new Promise((resolve, reject) => { const host = 'http://' + nextPrefix(); - setAPIModule(host, mockAPIModule); - mockAPIData({ - type: 'host', - host, - uri: '/test', - response: { - foo: 2, - }, - }); - let isSync = true; - sendAPIQuery( - { - resources: [host], - }, - { - type: 'custom', + try { + setAPIModule(host, mockAPIModule); + mockAPIData({ + type: 'host', + host, uri: '/test', - }, - (data, error) => { - expect(error).toBeUndefined(); - expect(data).toEqual({ + response: { foo: 2, - }); - expect(isSync).toBe(false); - fulfill(true); - } - ); + }, + }); + + sendAPIQuery( + { + resources: [host], + }, + { + type: 'custom', + uri: '/test', + }, + (data, error) => { + try { + expect(error).toBeUndefined(); + expect(data).toEqual({ + foo: 2, + }); + expect(isSync).toBe(false); + } catch (error) { + reject(error); + return; + } + resolve(true); + } + ); + } catch (error) { + reject(error); + } isSync = false; }); }); it('not_found response', () => { - return new Promise((fulfill) => { + return new Promise((resolve, reject) => { const prefix = nextPrefix(); - - mockAPIData({ - type: 'icons', - provider, - prefix, - icons: ['test1', 'test2'], - response: { - prefix, - icons: {}, - not_found: ['test1', 'test2'], - }, - }); - let isSync = true; - loadIcons( - [ - { - provider, + try { + mockAPIData({ + type: 'icons', + provider, + prefix, + icons: ['test1', 'test2'], + response: { prefix, - name: 'test1', + icons: {}, + not_found: ['test1', 'test2'], }, - ], - (loaded, missing, pending) => { - expect(isSync).toBe(false); - expect(loaded).toEqual([]); - expect(pending).toEqual([]); - expect(missing).toEqual([ + }); + + loadIcons( + [ { provider, prefix, name: 'test1', }, - ]); - fulfill(true); - } - ); + ], + (loaded, missing, pending) => { + try { + expect(isSync).toBe(false); + expect(loaded).toEqual([]); + expect(pending).toEqual([]); + expect(missing).toEqual([ + { + provider, + prefix, + name: 'test1', + }, + ]); + } catch (error) { + reject(error); + return; + } + resolve(true); + } + ); + } catch (error) { + reject(error); + } isSync = false; }); diff --git a/packages/utils/tests/parse-icons-test.ts b/packages/utils/tests/parse-icons-test.ts index 354367f..775df7d 100644 --- a/packages/utils/tests/parse-icons-test.ts +++ b/packages/utils/tests/parse-icons-test.ts @@ -319,4 +319,47 @@ describe('Testing parsing icon set', () => { parsedNames.sort((a, b) => a.localeCompare(b)); expect(parsedNames).toEqual(expectedNames); }); + + test('Bug test 1', () => { + // Names list + const names: string[] = ['test20', 'test21', 'BadName']; + + // Resolved data + const expected: Record = { + test20: { + body: '', + }, + test21: { + body: '', + }, + BadName: { + body: '', + }, + }; + + // Do stuff + expect( + parseIconSet( + { + prefix: 'api-mock-02', + icons: { + test20: { body: '' }, + test21: { body: '' }, + BadName: { body: '' }, + }, + }, + (name, data) => { + // Make sure name matches + expect(names.length).toBeGreaterThanOrEqual(1); + expect(name).toBe(names.shift()); + + // Check icon data + expect(data).toEqual(expected[name]); + } + ) + ).toEqual(['test20', 'test21', 'BadName']); + + // All names should have been parsed + expect(names).toEqual([]); + }); });