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
2026-01-23 22:19:07 +01:00
parent bf7efbf7e1
commit ca60ba148c
3 changed files with 460 additions and 20 deletions
@@ -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.
     *
@@ -55,6 +55,7 @@ export const macros = {

    // shorthand functions
    register: MacroRegistry.registerMacro.bind(MacroRegistry),
+    registerAlias: MacroRegistry.registerMacroAlias.bind(MacroRegistry),
 };

 /**
@@ -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;
+}