chore(core): refactor loader to check icon names only in API calls

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,53 +65,49 @@ function loadedNewIcons(storage: IconStorageWithAPI): void {
 	}
 }
 
+interface CheckIconNames {
+	valid: string[];
+	invalid: string[];
+}
 /**
- * Load icons
+ * Check icon names for API
  */
-function loadNewIcons(storage: IconStorageWithAPI, icons: string[]): void {
+function checkIconNamesForAPI(icons: string[]): CheckIconNames {
-	// Add icons to queue
+	const valid: string[] = [];
-	if (!storage.iconsToLoad) {
+	const invalid: string[] = [];
-		storage.iconsToLoad = icons;
-	} else {
-		storage.iconsToLoad = storage.iconsToLoad.concat(icons).sort();
-	}
 
-	// Trigger update on next tick, mering multiple synchronous requests into one asynchronous request
+	icons.forEach((name) => {
-	if (!storage.iconsQueueFlag) {
+		(name.match(matchIconName) ? valid : invalid).push(name);
-		storage.iconsQueueFlag = true;
+	});
-		setTimeout(() => {
+	return {
-			storage.iconsQueueFlag = false;
+		valid,
-			const { provider, prefix } = storage;
+		invalid,
+	};
+}
 
-			// Get icons and delete queue
+/**
-			const icons = storage.iconsToLoad;
+ * Parse loader response
-			delete storage.iconsToLoad;
+ */
-
+function parseLoaderResponse(
-			// Get API module
+	storage: IconStorageWithAPI,
-			let api: ReturnType<typeof getAPIModule>;
+	icons: string[],
-			if (!icons || !(api = getAPIModule(provider))) {
+	data: unknown
-				// No icons or no way to load icons!
+) {
-				return;
+	const fail = () => {
-			}
+		icons.forEach((name) => {
-
-			// Prepare parameters and run queries
-			const params = api.prepare(provider, prefix, icons);
-			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);
 		});
+	};
+
+	// Check for error
+	if (typeof data !== 'object' || !data) {
+		fail();
 	} else {
 		// Add icons to storage
 		try {
-							const parsed = addIconSet(
+			const parsed = addIconSet(storage, data as IconifyJSON);
-								storage,
-								data as IconifyJSON
-							);
 			if (!parsed.length) {
+				fail();
 				return;
 			}
 
@@ -128,6 +128,61 @@ function loadNewIcons(storage: IconStorageWithAPI, icons: string[]): void {
 
 	// Trigger update on next tick
 	loadedNewIcons(storage);
+}
+
+/**
+ * Load icons
+ */
+function loadNewIcons(storage: IconStorageWithAPI, icons: string[]): void {
+	// Add icons to queue
+	if (!storage.iconsToLoad) {
+		storage.iconsToLoad = icons;
+	} else {
+		storage.iconsToLoad = storage.iconsToLoad.concat(icons).sort();
+	}
+
+	// Trigger update on next tick, mering multiple synchronous requests into one asynchronous request
+	if (!storage.iconsQueueFlag) {
+		storage.iconsQueueFlag = true;
+		setTimeout(() => {
+			storage.iconsQueueFlag = false;
+			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
+			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, valid);
+			params.forEach((item) => {
+				sendAPIQuery(provider, item, (data) => {
+					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;
+		const list = newIcons[storage.provider][storage.prefix];
-		if (newIcons[provider][prefix].length) {
+		if (list.length) {
-			loadNewIcons(storage, newIcons[provider][prefix]);
+			loadNewIcons(storage, list);
 		}
 	});
 

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) {
 		//
 	}

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(
+		// After change to naming convention, icon name is valid and should be pending
-			false
+		// 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', () => {

packages/core/tests/api/mock-test.ts

@@ -29,9 +29,11 @@ describe('Testing mock API module', () => {
 
 	// Tests
 	it('404 response', () => {
-		return new Promise((fulfill) => {
+		return new Promise((resolve, reject) => {
 			const prefix = nextPrefix();
+			let isSync = true;
 
+			try {
 				mockAPIData({
 					type: 'icons',
 					provider,
@@ -40,8 +42,6 @@ describe('Testing mock API module', () => {
 					response: 404,
 				});
 
-			let isSync = true;
-
 				loadIcons(
 					[
 						{
@@ -51,6 +51,7 @@ describe('Testing mock API module', () => {
 						},
 					],
 					(loaded, missing, pending) => {
+						try {
 							expect(isSync).toBe(false);
 							expect(loaded).toEqual([]);
 							expect(pending).toEqual([]);
@@ -61,18 +62,27 @@ describe('Testing mock API module', () => {
 									name: 'test1',
 								},
 							]);
-					fulfill(true);
+						} 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();
+			let isSync = true;
 
+			try {
 				mockAPIData({
 					type: 'icons',
 					provider,
@@ -105,8 +115,21 @@ describe('Testing mock API module', () => {
 						},
 					},
 				});
-
+				// Data with invalid name: should not be requested from API because name is invalid
-			let isSync = true;
+				// Split from main data because otherwise it would load when other icons are requested
+				mockAPIData({
+					type: 'icons',
+					provider,
+					prefix,
+					response: {
+						prefix,
+						icons: {
+							BadName: {
+								body: '<g />',
+							},
+						},
+					},
+				});
 
 				loadIcons(
 					[
@@ -120,8 +143,19 @@ describe('Testing mock API module', () => {
 							prefix,
 							name: 'test20',
 						},
+						{
+							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([
@@ -137,20 +171,35 @@ describe('Testing mock API module', () => {
 								},
 							]);
 							expect(pending).toEqual([]);
-					expect(missing).toEqual([]);
+							expect(missing).toEqual([
-					fulfill(true);
+								{
+									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;
+			let callbackCounter = 0;
 
+			try {
 				mockAPIData({
 					type: 'icons',
 					provider,
@@ -187,8 +236,6 @@ describe('Testing mock API module', () => {
 					},
 				});
 
-			let callbackCounter = 0;
-
 				loadIcons(
 					[
 						{
@@ -204,6 +251,7 @@ describe('Testing mock API module', () => {
 					],
 					(loaded, missing, pending) => {
 						callbackCounter++;
+						try {
 							switch (callbackCounter) {
 								case 1:
 									// First load: only 'test10'
@@ -224,7 +272,6 @@ describe('Testing mock API module', () => {
 
 									// Send second response
 									expect(typeof next).toBe('function');
-							// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
 									next!();
 									break;
 
@@ -243,7 +290,7 @@ describe('Testing mock API module', () => {
 										},
 									]);
 									expect(missing).toEqual([]);
-							fulfill(true);
+									resolve(true);
 									break;
 
 								default:
@@ -251,17 +298,24 @@ describe('Testing mock API module', () => {
 										'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';
 
+			try {
 				// Mock data
 				mockAPIData({
 					type: 'icons',
@@ -276,6 +330,7 @@ describe('Testing mock API module', () => {
 						},
 					},
 					delay: (next) => {
+						try {
 							// Icon should not be loaded yet
 							const storage = getStorage(provider, prefix);
 							expect(iconInStorage(storage, name)).toBe(false);
@@ -285,8 +340,11 @@ describe('Testing mock API module', () => {
 
 							// Icon should be loaded now
 							expect(iconInStorage(storage, name)).toBe(true);
-
+						} catch (error) {
-					fulfill(true);
+							reject(error);
+							return;
+						}
+						resolve(true);
 					},
 				});
 
@@ -298,11 +356,17 @@ describe('Testing mock API module', () => {
 						name,
 					},
 				]);
+			} catch (error) {
+				reject(error);
+			}
 		});
 	});
 
 	it('Custom query', () => {
-		return new Promise((fulfill) => {
+		return new Promise((resolve, reject) => {
+			let isSync = true;
+
+			try {
 				mockAPIData({
 					type: 'custom',
 					provider,
@@ -312,8 +376,6 @@ describe('Testing mock API module', () => {
 					},
 				});
 
-			let isSync = true;
-
 				sendAPIQuery(
 					provider,
 					{
@@ -322,22 +384,33 @@ describe('Testing mock API module', () => {
 						uri: '/test',
 					},
 					(data, error) => {
+						try {
 							expect(error).toBeUndefined();
 							expect(data).toEqual({
 								foo: true,
 							});
 							expect(isSync).toBe(false);
-					fulfill(true);
+						} 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();
+			let isSync = true;
+
+			try {
 				setAPIModule(host, mockAPIModule);
 				mockAPIData({
 					type: 'host',
@@ -348,8 +421,6 @@ describe('Testing mock API module', () => {
 					},
 				});
 
-			let isSync = true;
-
 				sendAPIQuery(
 					{
 						resources: [host],
@@ -359,23 +430,33 @@ describe('Testing mock API module', () => {
 						uri: '/test',
 					},
 					(data, error) => {
+						try {
 							expect(error).toBeUndefined();
 							expect(data).toEqual({
 								foo: 2,
 							});
 							expect(isSync).toBe(false);
-					fulfill(true);
+						} 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();
+			let isSync = true;
 
+			try {
 				mockAPIData({
 					type: 'icons',
 					provider,
@@ -388,8 +469,6 @@ describe('Testing mock API module', () => {
 					},
 				});
 
-			let isSync = true;
-
 				loadIcons(
 					[
 						{
@@ -399,6 +478,7 @@ describe('Testing mock API module', () => {
 						},
 					],
 					(loaded, missing, pending) => {
+						try {
 							expect(isSync).toBe(false);
 							expect(loaded).toEqual([]);
 							expect(pending).toEqual([]);
@@ -409,9 +489,16 @@ describe('Testing mock API module', () => {
 									name: 'test1',
 								},
 							]);
-					fulfill(true);
+						} catch (error) {
+							reject(error);
+							return;
+						}
+						resolve(true);
 					}
 				);
+			} catch (error) {
+				reject(error);
+			}
 
 			isSync = false;
 		});

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<string, ExtendedIconifyIcon | null> = {
+			test20: {
+				body: '<g />',
+			},
+			test21: {
+				body: '<g />',
+			},
+			BadName: {
+				body: '<g />',
+			},
+		};
+
+		// Do stuff
+		expect(
+			parseIconSet(
+				{
+					prefix: 'api-mock-02',
+					icons: {
+						test20: { body: '<g />' },
+						test21: { body: '<g />' },
+						BadName: { body: '<g />' },
+					},
+				},
+				(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([]);
+	});
 });