From 62678f58ebf453e41ddf8f9204da9cb4f40dde29 Mon Sep 17 00:00:00 2001 From: Neil Fraser Date: Mon, 11 Mar 2024 15:52:31 +0100 Subject: [PATCH] Allow duplicate registration of values Also remove unused partially implemented case-sensitivity from registry. --- core/registry.ts | 113 +++++++++++++---------------------- tests/mocha/registry_test.js | 50 ++++++++++------ 2 files changed, 75 insertions(+), 88 deletions(-) diff --git a/core/registry.ts b/core/registry.ts index 3645a6fdf..8d7ab1844 100644 --- a/core/registry.ts +++ b/core/registry.ts @@ -24,6 +24,7 @@ import type {ToolboxItem} from './toolbox/toolbox_item.js'; import type {IPaster} from './interfaces/i_paster.js'; import type {ICopyData, ICopyable} from './interfaces/i_copyable.js'; import type {IConnectionPreviewer} from './interfaces/i_connection_previewer.js'; +import * as deprecation from './utils/deprecation.js'; /** * A map of maps. With the keys being the type and name of the class we are @@ -37,13 +38,6 @@ const typeMap: { } = Object.create(null); export const TEST_ONLY = {typeMap}; -/** - * A map of maps. With the keys being the type and caseless name of the class we - * are registring, and the value being the most recent cased name for that - * registration. - */ -const nameMap: {[key: string]: {[key: string]: string}} = Object.create(null); - /** * The string used to register the default class for a type of plugin. */ @@ -129,51 +123,27 @@ export function register( | AnyDuringMigration, opt_allowOverrides?: boolean, ): void { - if ( - (!(type instanceof Type) && typeof type !== 'string') || - `${type}`.trim() === '' - ) { - throw Error( - 'Invalid type "' + - type + - '". The type must be a' + - ' non-empty string or a Blockly.registry.Type.', - ); - } - type = `${type}`.toLowerCase(); - - if (typeof name !== 'string' || name.trim() === '') { - throw Error( - 'Invalid name "' + name + '". The name must be a' + ' non-empty string.', - ); - } - const caselessName = name.toLowerCase(); + type = normalizeName(`${type}`); + name = normalizeName(name); if (!registryItem) { throw Error('Can not register a null value'); } let typeRegistry = typeMap[type]; - let nameRegistry = nameMap[type]; // If the type registry has not been created, create it. if (!typeRegistry) { typeRegistry = typeMap[type] = Object.create(null); - nameRegistry = nameMap[type] = Object.create(null); } // Validate that the given class has all the required properties. validate(type, registryItem); // Don't throw an error if opt_allowOverrides is true. - if (!opt_allowOverrides && typeRegistry[caselessName]) { - throw Error( - 'Name "' + - caselessName + - '" with type "' + - type + - '" already registered.', - ); + // Don't throw an error if the registryItem is not changing. + if (!opt_allowOverrides && typeRegistry[name] && + typeRegistry[name] !== registryItem) { + throw Error(`Name "${name}" with type "${type}" already registered`); } - typeRegistry[caselessName] = registryItem; - nameRegistry[caselessName] = name; + typeRegistry[name] = registryItem; } /** @@ -202,22 +172,14 @@ function validate(type: string, registryItem: Function | AnyDuringMigration) { * @param name The plugin's name. (Ex. field_angle, geras) */ export function unregister(type: string | Type, name: string) { - type = `${type}`.toLowerCase(); - name = name.toLowerCase(); + type = normalizeName(`${type}`); + name = normalizeName(name); const typeRegistry = typeMap[type]; if (!typeRegistry || !typeRegistry[name]) { - console.warn( - 'Unable to unregister [' + - name + - '][' + - type + - '] from the ' + - 'registry.', - ); + console.warn(`Unable to unregister [${name}][${type}] from the registry`); return; } delete typeMap[type][name]; - delete nameMap[type][name]; } /** @@ -237,15 +199,13 @@ function getItem( name: string, opt_throwIfMissing?: boolean, ): (new (...p1: AnyDuringMigration[]) => T) | null | AnyDuringMigration { - type = `${type}`.toLowerCase(); - name = name.toLowerCase(); + type = normalizeName(`${type}`); + name = normalizeName(name); const typeRegistry = typeMap[type]; if (!typeRegistry || !typeRegistry[name]) { - const msg = 'Unable to find [' + name + '][' + type + '] in the registry.'; + const msg = `Unable to find [${name}][${type}] in the registry.`; if (opt_throwIfMissing) { - throw new Error( - msg + ' You must require or register a ' + type + ' plugin.', - ); + throw Error(`${msg} You must require or register a ${type} plugin`); } else { console.warn(msg); } @@ -265,8 +225,8 @@ function getItem( * otherwise. */ export function hasItem(type: string | Type, name: string): boolean { - type = `${type}`.toLowerCase(); - name = name.toLowerCase(); + type = normalizeName(`${type}`); + name = normalizeName(name); const typeRegistry = typeMap[type]; if (!typeRegistry) { return false; @@ -274,6 +234,21 @@ export function hasItem(type: string | Type, name: string): boolean { return !!typeRegistry[name]; } +/** + * Normalize a string to be used as a key. By default, lowercase and trimmed. + * + * @param name The key's name. (Ex. 'FiELd_AnGlE ') + * @returns A normalized string. (Ex. 'field_angle') + */ +function normalizeName(name: string): string { + // Do we need this type checking? We are using TypeScript after all. + // But there's a unit test that depends on throwing this error. + if (typeof name !== 'string') throw Error('Invalid name'); + name = name.toLowerCase().trim(); + if (!name) throw Error('Empty name'); + return name; +} + /** * Gets the class for the given name and type. * @@ -316,18 +291,24 @@ export function getObject( * Returns a map of items registered with the given type. * * @param type The type of the plugin. (e.g. Category) - * @param opt_cased Whether or not to return a map with cased keys (rather than - * caseless keys). False by default. + * @param opt_cased Deprecated. * @param opt_throwIfMissing Whether or not to throw an error if we are unable * to find the object. False by default. * @returns A map of objects with the given type, or null if none exists. */ export function getAllItems( type: string | Type, - opt_cased?: boolean, + opt_cased?: any, opt_throwIfMissing?: boolean, ): {[key: string]: T | null | (new (...p1: AnyDuringMigration[]) => T)} | null { - type = `${type}`.toLowerCase(); + if (typeof opt_cased !== 'undefined') { + deprecation.warn( + 'Blockly.registry.getAllItems', + 'v11', + 'v12', + ); + } + type = normalizeName(`${type}`); const typeRegistry = typeMap[type]; if (!typeRegistry) { const msg = `Unable to find [${type}] in the registry.`; @@ -338,15 +319,7 @@ export function getAllItems( } return null; } - if (!opt_cased) { - return typeRegistry; - } - const nameRegistry = nameMap[type]; - const casedRegistry = Object.create(null); - for (const key of Object.keys(typeRegistry)) { - casedRegistry[nameRegistry[key]] = typeRegistry[key]; - } - return casedRegistry; + return typeRegistry; } /** diff --git a/tests/mocha/registry_test.js b/tests/mocha/registry_test.js index fd37c6e69..dca776050 100644 --- a/tests/mocha/registry_test.js +++ b/tests/mocha/registry_test.js @@ -34,21 +34,30 @@ suite('Registry', function () { test('Empty String Key', function () { chai.assert.throws(function () { - Blockly.registry.register('test', '', TestClass); - }, 'Invalid name'); + Blockly.registry.register('test', ' ', TestClass); + }, 'Empty name'); }); test('Class as Key', function () { chai.assert.throws(function () { - Blockly.registry.register('test', TestClass, ''); + Blockly.registry.register('test', TestClass, TestClass); }, 'Invalid name'); }); test('Overwrite a Key', function () { Blockly.registry.register('test', 'test_name', TestClass); + // Duplicate registration of the same object is ok. + Blockly.registry.register('test', 'test_name', TestClass); + // Registering some other value is not ok. chai.assert.throws(function () { - Blockly.registry.register('test', 'test_name', TestClass); + Blockly.registry.register('test', 'test_name', {}); }, 'already registered'); + // Changing the case or padding doesn't help. + chai.assert.throws(function () { + Blockly.registry.register('test', ' test_NAME ', {}); + }, 'already registered'); + // But it's ok if explicitly allowed with 'true'. + Blockly.registry.register('test', 'test_name', {}, true); }); test('Null Value', function () { @@ -77,7 +86,7 @@ suite('Registry', function () { }); }); - suite('Case', function () { + suite('Name normalization', function () { test('Caseless type', function () { chai.assert.isTrue(Blockly.registry.hasItem('TEST', 'test_name')); }); @@ -85,6 +94,10 @@ suite('Registry', function () { test('Caseless name', function () { chai.assert.isTrue(Blockly.registry.hasItem('test', 'TEST_NAME')); }); + + test('Padded name', function () { + chai.assert.isTrue(Blockly.registry.hasItem(' test ', 'TEST_NAME')); + }); }); }); @@ -119,7 +132,7 @@ suite('Registry', function () { }); }); - suite('Case', function () { + suite('Name normalization', function () { test('Caseless type', function () { chai.assert.isNotNull(Blockly.registry.getClass('TEST', 'test_name')); }); @@ -127,6 +140,10 @@ suite('Registry', function () { test('Caseless name', function () { chai.assert.isNotNull(Blockly.registry.getClass('test', 'TEST_NAME')); }); + + test('Padded name', function () { + chai.assert.isTrue(Blockly.registry.hasItem(' test ', 'TEST_NAME')); + }); }); }); @@ -161,7 +178,7 @@ suite('Registry', function () { }); }); - suite('Case', function () { + suite('Name normalization', function () { test('Caseless type', function () { chai.assert.isNotNull(Blockly.registry.getObject('TEST', 'test_name')); }); @@ -169,6 +186,10 @@ suite('Registry', function () { test('Caseless name', function () { chai.assert.isNotNull(Blockly.registry.getObject('test', 'TEST_NAME')); }); + + test('Padded name', function () { + chai.assert.isTrue(Blockly.registry.hasItem(' test ', 'TEST_NAME')); + }); }); }); @@ -194,7 +215,7 @@ suite('Registry', function () { test('Throw if missing', function () { chai.assert.throws(function () { - Blockly.registry.getAllItems('bad_type', false, true); + Blockly.registry.getAllItems('bad_type', undefined, true); }); }); @@ -202,18 +223,11 @@ suite('Registry', function () { chai.assert.isNotNull(Blockly.registry.getAllItems('TEST')); }); - test('Respect name case', function () { - chai.assert.deepEqual(Blockly.registry.getAllItems('test', true), { - 'test_name': {}, - 'casedNAME': {}, - }); - }); - - test('Respect overwriting name case', function () { + test('Overwriting name case', function () { Blockly.registry.register('test', 'CASEDname', {}, true); - chai.assert.deepEqual(Blockly.registry.getAllItems('test', true), { + chai.assert.deepEqual(Blockly.registry.getAllItems('test'), { 'test_name': {}, - 'CASEDname': {}, + 'casedname': {}, }); }); });