From ca60ba148c39425f7ab682c7592382cfd0efc715 Mon Sep 17 00:00:00 2001 From: Wolfsblvt Date: Fri, 23 Jan 2026 22:19:07 +0100 Subject: [PATCH] chore(macros): Allow registration of aliases for existing macros (#5053) - Added `registerMacroAlias()` public method to register aliases for existing macros - Extracted shared registration logic into private `#registerMacroEntry()` helper - Refactored `registerMacro()` to use `#registerMacroEntry()` for both primary macros and aliases - Alias registration validates name format, prevents self-aliasing, and resolves alias chains to primary definition - Aliases inherit handler and metadata from target but has source of the registration caller --- public/scripts/macros/engine/MacroRegistry.js | 112 +++++- public/scripts/macros/macro-system.js | 1 + tests/frontend/MacroRegistry.e2e.js | 367 +++++++++++++++++- 3 files changed, 460 insertions(+), 20 deletions(-) diff --git a/public/scripts/macros/engine/MacroRegistry.js b/public/scripts/macros/engine/MacroRegistry.js index cbfee3e92..450cdd932 100644 --- a/public/scripts/macros/engine/MacroRegistry.js +++ b/public/scripts/macros/engine/MacroRegistry.js @@ -200,11 +200,6 @@ class MacroRegistry { name = typeof name === 'string' ? name.trim() : String(name); try { - const nameKey = name.toLowerCase(); - if (this.#macros.has(nameKey)) { - logMacroRegisterWarning({ macroName: name, message: `Macro "${name}" is already registered and will be overwritten.` }); - } - // Detect extension/third-party status from call stack const { isExtension, isThirdParty, source } = detectMacroSource(); @@ -213,22 +208,12 @@ class MacroRegistry { source: { name: source, isExtension, isThirdParty }, }); - this.#macros.set(nameKey, definition); + // Register the primary macro + this.#registerMacroEntry(name, definition); // Register alias entries pointing to the same definition for (const { alias, visible } of definition.aliases) { - const aliasKey = alias.toLowerCase(); - if (this.#macros.has(aliasKey)) { - logMacroRegisterWarning({ macroName: name, message: `Alias "${alias}" for macro "${name}" overwrites an existing macro.` }); - } - /** @type {MacroDefinition} */ - const aliasEntry = { - ...definition, - name: alias, // The lookup name is the alias (preserves original casing for display) - aliasOf: name, - aliasVisible: visible, - }; - this.#macros.set(aliasKey, aliasEntry); + this.#registerMacroEntry(alias, definition, { primaryMacroName: name, aliasVisible: visible }); } return definition; @@ -242,6 +227,97 @@ class MacroRegistry { } } + /** + * Registers an alias for an existing macro. + * The alias will point to the same handler and metadata as the original macro. + * Errors during registration are caught and logged, the alias will not be registered, and the function returns false. + * + * @param {string} targetMacroName - The name of the existing macro to create an alias for. + * @param {string} aliasName - The alias name (identifier). + * @param {Object} [options] - Alias registration options. + * @param {boolean} [options.visible=true] - Whether this alias appears in documentation/autocomplete. + * @returns {boolean} True if the alias was registered successfully, false if registration failed. + */ + registerMacroAlias(targetMacroName, aliasName, { visible = true } = {}) { + // Extract names early for error logging + targetMacroName = typeof targetMacroName === 'string' ? targetMacroName.trim() : String(targetMacroName); + aliasName = typeof aliasName === 'string' ? aliasName.trim() : String(aliasName); + + try { + // Validate alias name + if (!isIdentifierValid(aliasName)) { + throw new Error(`Alias name "${aliasName}" is invalid. Must start with a letter, followed by alphanumeric characters or hyphens.`); + } + + // Check that alias is not the same as target (case insensitive) + if (aliasName.toLowerCase() === targetMacroName.toLowerCase()) { + throw new Error(`Alias name "${aliasName}" cannot be the same as the target macro name (case insensitive).`); + } + + // Check that target macro exists + const targetDefinition = this.getMacro(targetMacroName); + if (!targetDefinition) { + throw new Error(`Target macro "${targetMacroName}" is not registered.`); + } + + // Get the primary definition (in case target is itself an alias) + const primaryDefinition = targetDefinition.aliasOf ? this.getMacro(targetDefinition.aliasOf) : targetDefinition; + if (!primaryDefinition) { + throw new Error(`Could not resolve primary definition for target macro "${targetMacroName}".`); + } + + // Detect extension/third-party status from call stack + const { isExtension, isThirdParty, source } = detectMacroSource(); + + // Create alias definition with source detection + const aliasDefinition = { + ...primaryDefinition, + source: { name: source, isExtension, isThirdParty }, + }; + + // Register the alias using the shared utility + this.#registerMacroEntry(aliasName, aliasDefinition, { primaryMacroName: primaryDefinition.name, aliasVisible: visible }); + + return true; + } catch (error) { + logMacroRegisterError({ + message: `Failed to register alias "${aliasName}" for macro "${targetMacroName}". The alias will not be available.`, + macroName: aliasName, + error, + }); + return false; + } + } + + /** + * Shared utility for registering macro entries (primary or alias). + * + * @param {string} name - The registration name (primary macro or alias). + * @param {MacroDefinition} definition - The definition to register. + * @param {Object} [options={}] - Options for alias registration. + * @param {string} [options.primaryMacroName=null] - For aliases, the primary macro name. + * @param {boolean} [options.aliasVisible=null] - For aliases, visibility flag. + */ + #registerMacroEntry(name, definition, { primaryMacroName = null, aliasVisible = null } = {}) { + const nameKey = name.toLowerCase(); + + if (this.#macros.has(nameKey)) { + const warningType = primaryMacroName ? `Alias "${name}" for macro "${primaryMacroName}"` : `Macro "${name}"`; + const warningMessage = primaryMacroName ? 'overwrites an existing macro.' : 'is already registered and will be overwritten.'; + logMacroRegisterWarning({ macroName: primaryMacroName || name, message: `${warningType} ${warningMessage}` }); + } + + /** @type {MacroDefinition} */ + const entry = primaryMacroName ? { + ...definition, + name: name, // The lookup name is the alias (preserves original casing for display) + aliasOf: primaryMacroName, + aliasVisible: aliasVisible, + } : definition; + + this.#macros.set(nameKey, entry); + } + /** * Unregisters a macro. * diff --git a/public/scripts/macros/macro-system.js b/public/scripts/macros/macro-system.js index 73f7f08f2..7ff9dbda8 100644 --- a/public/scripts/macros/macro-system.js +++ b/public/scripts/macros/macro-system.js @@ -55,6 +55,7 @@ export const macros = { // shorthand functions register: MacroRegistry.registerMacro.bind(MacroRegistry), + registerAlias: MacroRegistry.registerMacroAlias.bind(MacroRegistry), }; /** diff --git a/tests/frontend/MacroRegistry.e2e.js b/tests/frontend/MacroRegistry.e2e.js index 971837442..fede29b08 100644 --- a/tests/frontend/MacroRegistry.e2e.js +++ b/tests/frontend/MacroRegistry.e2e.js @@ -5,7 +5,7 @@ test.describe('MacroRegistry', () => { // Currently this test suits runs without ST context. Enable, if ever needed test.beforeEach(testSetup.awaitST); - test.describe('valid', () => { + test.describe('register valid', () => { test('should register a macro with valid options', async ({ page }) => { const result = await page.evaluate(async () => { /** @type {import('../../public/scripts/macros/engine/MacroRegistry.js')} */ @@ -42,7 +42,7 @@ test.describe('MacroRegistry', () => { }); }); - test.describe('reject', () => { + test.describe('register reject', () => { test('should reject invalid macro name', async ({ page }) => { const result = await registerMacroAndCaptureErrors(page, { macroName: ' ', @@ -291,6 +291,322 @@ test.describe('MacroRegistry', () => { expect(registrationError?.errorMessage).toContain('is invalid'); }); }); + + test.describe('registerMacroAlias', () => { + test.describe('valid', () => { + test('should register an alias for an existing macro', async ({ page }) => { + const result = await page.evaluate(async () => { + /** @type {import('../../public/scripts/macros/engine/MacroRegistry.js')} */ + const { MacroRegistry } = await import('./scripts/macros/engine/MacroRegistry.js'); + + // Clean up any existing registrations + MacroRegistry.unregisterMacro('alias-target'); + MacroRegistry.unregisterMacro('my-alias'); + + // Register target macro + MacroRegistry.registerMacro('alias-target', { + description: 'Target macro for alias test', + handler: () => 'target-result', + }); + + // Register alias + const success = MacroRegistry.registerMacroAlias('alias-target', 'my-alias'); + + const aliasDef = MacroRegistry.getMacro('my-alias'); + const targetDef = MacroRegistry.getMacro('alias-target'); + + return { + success, + aliasName: aliasDef?.name, + aliasOf: aliasDef?.aliasOf, + aliasVisible: aliasDef?.aliasVisible, + targetName: targetDef?.name, + sameHandler: aliasDef?.handler === targetDef?.handler, + }; + }); + + expect(result.success).toBe(true); + expect(result.aliasName).toBe('my-alias'); + expect(result.aliasOf).toBe('alias-target'); + expect(result.aliasVisible).toBe(true); + expect(result.targetName).toBe('alias-target'); + expect(result.sameHandler).toBe(true); + }); + + test('should register alias with visible=false option', async ({ page }) => { + const result = await page.evaluate(async () => { + /** @type {import('../../public/scripts/macros/engine/MacroRegistry.js')} */ + const { MacroRegistry } = await import('./scripts/macros/engine/MacroRegistry.js'); + + MacroRegistry.unregisterMacro('alias-target-hidden'); + MacroRegistry.unregisterMacro('hidden-alias'); + + MacroRegistry.registerMacro('alias-target-hidden', { + description: 'Target macro', + handler: () => 'result', + }); + + const success = MacroRegistry.registerMacroAlias('alias-target-hidden', 'hidden-alias', { visible: false }); + const aliasDef = MacroRegistry.getMacro('hidden-alias'); + + return { + success, + aliasVisible: aliasDef?.aliasVisible, + }; + }); + + expect(result.success).toBe(true); + expect(result.aliasVisible).toBe(false); + }); + + test('should resolve alias of alias to primary definition', async ({ page }) => { + const result = await page.evaluate(async () => { + /** @type {import('../../public/scripts/macros/engine/MacroRegistry.js')} */ + const { MacroRegistry } = await import('./scripts/macros/engine/MacroRegistry.js'); + + MacroRegistry.unregisterMacro('primary-macro'); + MacroRegistry.unregisterMacro('first-alias'); + MacroRegistry.unregisterMacro('second-alias'); + + // Register primary macro + MacroRegistry.registerMacro('primary-macro', { + description: 'Primary macro', + handler: () => 'primary-result', + }); + + // Register first alias + MacroRegistry.registerMacroAlias('primary-macro', 'first-alias'); + + // Register alias of alias (should resolve to primary) + const success = MacroRegistry.registerMacroAlias('first-alias', 'second-alias'); + + const secondAliasDef = MacroRegistry.getMacro('second-alias'); + + return { + success, + aliasOf: secondAliasDef?.aliasOf, + }; + }); + + expect(result.success).toBe(true); + // Should point to primary, not to the intermediate alias + expect(result.aliasOf).toBe('primary-macro'); + }); + + test('should have independent source for alias', async ({ page }) => { + const result = await page.evaluate(async () => { + /** @type {import('../../public/scripts/macros/engine/MacroRegistry.js')} */ + const { MacroRegistry } = await import('./scripts/macros/engine/MacroRegistry.js'); + + MacroRegistry.unregisterMacro('source-target'); + MacroRegistry.unregisterMacro('source-alias'); + + MacroRegistry.registerMacro('source-target', { + description: 'Target', + handler: () => '', + }); + + MacroRegistry.registerMacroAlias('source-target', 'source-alias'); + + const targetDef = MacroRegistry.getMacro('source-target'); + const aliasDef = MacroRegistry.getMacro('source-alias'); + + return { + // Both should have source objects + targetHasSource: !!targetDef?.source, + aliasHasSource: !!aliasDef?.source, + // The alias has its own source object (not shared reference) + sourcesAreDifferentObjects: targetDef?.source !== aliasDef?.source, + }; + }); + + expect(result.targetHasSource).toBe(true); + expect(result.aliasHasSource).toBe(true); + expect(result.sourcesAreDifferentObjects).toBe(true); + }); + + test('should be case-insensitive for lookup', async ({ page }) => { + const result = await page.evaluate(async () => { + /** @type {import('../../public/scripts/macros/engine/MacroRegistry.js')} */ + const { MacroRegistry } = await import('./scripts/macros/engine/MacroRegistry.js'); + + MacroRegistry.unregisterMacro('case-target'); + MacroRegistry.unregisterMacro('CaseAlias'); + + MacroRegistry.registerMacro('case-target', { + handler: () => '', + }); + + MacroRegistry.registerMacroAlias('case-target', 'CaseAlias'); + + return { + foundLowercase: !!MacroRegistry.getMacro('casealias'), + foundUppercase: !!MacroRegistry.getMacro('CASEALIAS'), + foundMixed: !!MacroRegistry.getMacro('CaseAlias'), + }; + }); + + expect(result.foundLowercase).toBe(true); + expect(result.foundUppercase).toBe(true); + expect(result.foundMixed).toBe(true); + }); + }); + + test.describe('reject', () => { + test('should reject invalid alias name', async ({ page }) => { + const result = await registerAliasAndCaptureErrors(page, { + targetMacroName: 'random', + aliasName: '123-invalid', + }); + + expect(result.success).toBe(false); + expect(result.errors.length).toBeGreaterThan(0); + + const registrationError = result.errors.find(e => e.text.includes('[Macro] Registration Error:')); + expect(registrationError).toBeTruthy(); + expect(registrationError?.text).toContain('Failed to register alias "123-invalid"'); + expect(registrationError?.errorMessage).toContain('is invalid'); + }); + + test('should reject alias same as target name (case insensitive)', async ({ page }) => { + const result = await registerAliasAndCaptureErrors(page, { + targetMacroName: 'random', + aliasName: 'RANDOM', + }); + + expect(result.success).toBe(false); + expect(result.errors.length).toBeGreaterThan(0); + + const registrationError = result.errors.find(e => e.text.includes('[Macro] Registration Error:')); + expect(registrationError).toBeTruthy(); + expect(registrationError?.errorMessage).toContain('cannot be the same as the target macro name'); + }); + + test('should reject alias for non-existent target macro', async ({ page }) => { + const result = await registerAliasAndCaptureErrors(page, { + targetMacroName: 'non-existent-macro-xyz', + aliasName: 'my-alias', + }); + + expect(result.success).toBe(false); + expect(result.errors.length).toBeGreaterThan(0); + + const registrationError = result.errors.find(e => e.text.includes('[Macro] Registration Error:')); + expect(registrationError).toBeTruthy(); + expect(registrationError?.errorMessage).toContain('is not registered'); + }); + + test('should reject alias with special characters', async ({ page }) => { + const result = await registerAliasAndCaptureErrors(page, { + targetMacroName: 'random', + aliasName: 'alias@name', + }); + + expect(result.success).toBe(false); + const registrationError = result.errors.find(e => e.text.includes('[Macro] Registration Error:')); + expect(registrationError?.errorMessage).toContain('is invalid'); + }); + + test('should reject alias starting with hyphen', async ({ page }) => { + const result = await registerAliasAndCaptureErrors(page, { + targetMacroName: 'random', + aliasName: '-alias', + }); + + expect(result.success).toBe(false); + const registrationError = result.errors.find(e => e.text.includes('[Macro] Registration Error:')); + expect(registrationError?.errorMessage).toContain('is invalid'); + }); + }); + + test.describe('warnings', () => { + test('should warn when alias overwrites existing macro', async ({ page }) => { + const result = await page.evaluate(async () => { + /** @type {string[]} */ + const warnings = []; + const originalWarn = console.warn; + + console.warn = (...args) => { + warnings.push(args.map(a => String(a)).join(' ')); + }; + + try { + /** @type {import('../../public/scripts/macros/engine/MacroRegistry.js')} */ + const { MacroRegistry } = await import('./scripts/macros/engine/MacroRegistry.js'); + + MacroRegistry.unregisterMacro('overwrite-target'); + MacroRegistry.unregisterMacro('overwrite-existing'); + + // Register target macro + MacroRegistry.registerMacro('overwrite-target', { + handler: () => 'target', + }); + + // Register a macro that will be overwritten + MacroRegistry.registerMacro('overwrite-existing', { + handler: () => 'existing', + }); + + // Register alias that overwrites existing macro + const success = MacroRegistry.registerMacroAlias('overwrite-target', 'overwrite-existing'); + + return { success, warnings }; + } finally { + console.warn = originalWarn; + } + }); + + expect(result.success).toBe(true); + const overwriteWarning = result.warnings.find(w => + w.includes('overwrites an existing macro') && w.includes('overwrite-existing'), + ); + expect(overwriteWarning).toBeTruthy(); + }); + + test('should warn when alias overwrites another alias', async ({ page }) => { + const result = await page.evaluate(async () => { + /** @type {string[]} */ + const warnings = []; + const originalWarn = console.warn; + + console.warn = (...args) => { + warnings.push(args.map(a => String(a)).join(' ')); + }; + + try { + /** @type {import('../../public/scripts/macros/engine/MacroRegistry.js')} */ + const { MacroRegistry } = await import('./scripts/macros/engine/MacroRegistry.js'); + + MacroRegistry.unregisterMacro('alias-warn-target1'); + MacroRegistry.unregisterMacro('alias-warn-target2'); + MacroRegistry.unregisterMacro('shared-alias-name'); + + // Register two target macros + MacroRegistry.registerMacro('alias-warn-target1', { handler: () => '1' }); + MacroRegistry.registerMacro('alias-warn-target2', { handler: () => '2' }); + + // Register first alias + MacroRegistry.registerMacroAlias('alias-warn-target1', 'shared-alias-name'); + + // Clear warnings from first registration + warnings.length = 0; + + // Register second alias with same name (should warn) + MacroRegistry.registerMacroAlias('alias-warn-target2', 'shared-alias-name'); + + return { warnings }; + } finally { + console.warn = originalWarn; + } + }); + + const overwriteWarning = result.warnings.find(w => + w.includes('overwrites an existing macro') && w.includes('shared-alias-name'), + ); + expect(overwriteWarning).toBeTruthy(); + }); + }); + }); }); /** @@ -354,3 +670,50 @@ async function registerMacroAndCaptureErrors(page, { macroName, options }) { return result; } + +/** + * @param {import('@playwright/test').Page} page + * @param {{ targetMacroName: string, aliasName: string, options?: { visible?: boolean } }} params + * @returns {Promise<{ success: boolean, errors: CapturedConsoleError[] }>} + */ +async function registerAliasAndCaptureErrors(page, { targetMacroName, aliasName, options = {} }) { + const result = await page.evaluate(async ({ targetMacroName, aliasName, options }) => { + /** @type {CapturedConsoleError[]} */ + const errors = []; + const originalError = console.error; + + console.error = (...args) => { + const text = args + .map(a => (typeof a === 'string' ? a : (a instanceof Error ? `Error: ${a.message}` : ''))) + .filter(Boolean) + .join(' '); + + /** @type {string|null} */ + let errorMessage = null; + for (const a of args) { + if (a instanceof Error) { + errorMessage ??= a.message; + continue; + } + if (a && typeof a === 'object' && 'error' in a && a.error instanceof Error) { + errorMessage ??= a.error.message; + } + } + + errors.push({ text, errorMessage }); + }; + + try { + /** @type {import('../../public/scripts/macros/engine/MacroRegistry.js')} */ + const { MacroRegistry } = await import('./scripts/macros/engine/MacroRegistry.js'); + + // Registering an invalid alias does not throw. It returns false and logs an error. + const success = MacroRegistry.registerMacroAlias(targetMacroName, aliasName, options); + return { success, errors }; + } finally { + console.error = originalError; + } + }, { targetMacroName, aliasName, options }); + + return result; +}