From 5a3b183ea5209e8930e3cb89cff8abbbaede1feb Mon Sep 17 00:00:00 2001 From: MarcosBrendonDePaula Date: Sat, 14 Mar 2026 19:36:31 -0300 Subject: [PATCH] refactor: add registerSync() to PluginRegistry, eliminate registryInternals hack (#77) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add typed public methods (registerSync, refreshLoadOrder, getPluginsMap) to PluginRegistry so the framework no longer needs to bypass private fields via `as unknown as PluginRegistryInternals`. Remove the PluginRegistryInternals interface and all 12 encapsulation violations from server.ts. Add 20 unit tests covering sync registration, dependency chains, diamond deps, priority ordering, circular detection, and mixed priority+dependency scenarios. Closes #77 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- core/framework/server.ts | 59 +--- core/plugins/registry.ts | 50 ++++ .../core/plugins/registry-sync-api.test.ts | 271 ++++++++++++++++++ 3 files changed, 326 insertions(+), 54 deletions(-) create mode 100644 tests/unit/core/plugins/registry-sync-api.test.ts diff --git a/core/framework/server.ts b/core/framework/server.ts index 0aa9020..7d811f1 100644 --- a/core/framework/server.ts +++ b/core/framework/server.ts @@ -14,17 +14,6 @@ import { createHash } from "crypto" import { createPluginUtils } from "@core/plugins/config" import type { Plugin } from "@core/plugins" -/** - * Interface for accessing private PluginRegistry members. - * The framework needs direct access for synchronous plugin registration. - */ -interface PluginRegistryInternals { - plugins: Map - dependencies: Map - loadOrder: string[] - updateLoadOrder(): void -} - export class FluxStackFramework { private app: Elysia private context: FluxStackContext @@ -35,11 +24,6 @@ export class FluxStackFramework { private requestTimings: Map = new Map() private _originalStderrWrite?: typeof process.stderr.write - /** Access registry internals for synchronous plugin management */ - private get registryInternals(): PluginRegistryInternals { - return this.pluginRegistry as unknown as PluginRegistryInternals - } - /** Access typed config from context (config is stored as unknown to avoid circular deps) */ private get cfg(): import('@config').FluxStackConfig { return this.context.config as import('@config').FluxStackConfig @@ -188,23 +172,12 @@ export class FluxStackFramework { const discoveredPlugins = this.pluginManager.getRegistry().getAll() for (const plugin of discoveredPlugins) { if (!this.pluginRegistry.has(plugin.name)) { - // Register in main registry (synchronously, will call setup in start()) - this.registryInternals.plugins.set(plugin.name, plugin) - if (plugin.dependencies) { - this.registryInternals.dependencies.set(plugin.name, plugin.dependencies) - } + this.pluginRegistry.registerSync(plugin) } } - // Update load order - try { - this.registryInternals.updateLoadOrder() - } catch { - // Fallback: create basic load order - const plugins = this.registryInternals.plugins - const loadOrder = Array.from(plugins.keys()) - this.registryInternals.loadOrder = loadOrder - } + // Refresh load order (falls back to insertion-order on failure) + this.pluginRegistry.refreshLoadOrder() // Execute onConfigLoad hooks for all plugins const configLoadContext = { @@ -680,29 +653,7 @@ export class FluxStackFramework { use(plugin: Plugin) { try { - // Use the registry's public register method, but don't await it since we need sync operation - if (this.pluginRegistry.has(plugin.name)) { - throw new Error(`Plugin '${plugin.name}' is already registered`) - } - - // Store plugin without calling setup - setup will be called in start() - // We need to manually set the plugin since register() is async but we need sync - this.registryInternals.plugins.set(plugin.name, plugin) - - // Update dependencies tracking - if ((plugin as FluxStack.Plugin).dependencies) { - this.registryInternals.dependencies.set(plugin.name, (plugin as FluxStack.Plugin).dependencies!) - } - - // Update load order by calling the private method - try { - this.registryInternals.updateLoadOrder() - } catch { - // Fallback: create basic load order - const plugins = this.registryInternals.plugins - const loadOrder = Array.from(plugins.keys()) - this.registryInternals.loadOrder = loadOrder - } + this.pluginRegistry.registerSync(plugin as FluxStack.Plugin) logger.debug(`Plugin '${plugin.name}' registered`, { version: (plugin as FluxStack.Plugin).version, @@ -733,7 +684,7 @@ export class FluxStackFramework { await this.initializeAutomaticPlugins() // Validate plugin dependencies before starting - const plugins = this.registryInternals.plugins + const plugins = this.pluginRegistry.getPluginsMap() for (const [pluginName, plugin] of plugins) { if (plugin.dependencies) { for (const depName of plugin.dependencies) { diff --git a/core/plugins/registry.ts b/core/plugins/registry.ts index 5f776bd..f820de6 100644 --- a/core/plugins/registry.ts +++ b/core/plugins/registry.ts @@ -237,6 +237,56 @@ export class PluginRegistry { return this.plugins.has(name) } + /** + * Register a plugin synchronously (no async hooks). + * + * Used by the framework to add plugins via .use() and during automatic + * plugin discovery, where the full async register() flow (which fires + * onPluginRegister hooks) is not needed — setup hooks run later in start(). + */ + registerSync(plugin: FluxStackPlugin): void { + if (this.plugins.has(plugin.name)) { + throw new FluxStackError( + `Plugin '${plugin.name}' is already registered`, + 'PLUGIN_ALREADY_REGISTERED', + 400 + ) + } + + this.validatePlugin(plugin) + this.plugins.set(plugin.name, plugin) + + if (plugin.dependencies) { + this.dependencies.set(plugin.name, plugin.dependencies) + } + + this.updateLoadOrder() + } + + /** + * Refresh the load order. + * + * Falls back to insertion-order if the topological sort fails + * (e.g. unresolvable external dependency listed but not yet registered). + */ + refreshLoadOrder(): void { + try { + this.updateLoadOrder() + } catch { + this.loadOrder = Array.from(this.plugins.keys()) + } + } + + /** + * Return a read-only snapshot of the internal plugin map. + * + * Allows the framework to iterate over registered plugins for dependency + * validation without reaching into private fields. + */ + getPluginsMap(): ReadonlyMap { + return this.plugins + } + /** * Check which dependencies are missing from main package.json */ diff --git a/tests/unit/core/plugins/registry-sync-api.test.ts b/tests/unit/core/plugins/registry-sync-api.test.ts new file mode 100644 index 0000000..fe89ef5 --- /dev/null +++ b/tests/unit/core/plugins/registry-sync-api.test.ts @@ -0,0 +1,271 @@ +/** + * Tests for issue #77: PluginRegistry public sync API + * + * Validates registerSync(), refreshLoadOrder(), and getPluginsMap() — the + * typed public methods that replace the old `as unknown as PluginRegistryInternals` + * pattern in FluxStackFramework. + */ + +import { describe, it, expect } from 'vitest' +import { PluginRegistry } from '@core/plugins/registry' +import type { FluxStack } from '@core/plugins/types' + +function makePlugin(overrides: Partial & { name: string }): FluxStack.Plugin { + return { ...overrides } +} + +describe('PluginRegistry sync API (#77)', () => { + describe('registerSync()', () => { + it('should register a plugin synchronously', () => { + const registry = new PluginRegistry() + const plugin = makePlugin({ name: 'sync-test' }) + + registry.registerSync(plugin) + + expect(registry.has('sync-test')).toBe(true) + expect(registry.get('sync-test')).toBe(plugin) + }) + + it('should track dependencies', () => { + const registry = new PluginRegistry() + const base = makePlugin({ name: 'base' }) + const child = makePlugin({ name: 'child', dependencies: ['base'] }) + + registry.registerSync(base) + registry.registerSync(child) + + expect(registry.getDependencies('child')).toEqual(['base']) + expect(registry.getDependents('base')).toEqual(['child']) + }) + + it('should update load order respecting dependencies', () => { + const registry = new PluginRegistry() + const base = makePlugin({ name: 'base' }) + const child = makePlugin({ name: 'child', dependencies: ['base'] }) + + // Register child first, then base + registry.registerSync(base) + registry.registerSync(child) + + const order = registry.getLoadOrder() + expect(order.indexOf('base')).toBeLessThan(order.indexOf('child')) + }) + + it('should throw when registering duplicate plugin', () => { + const registry = new PluginRegistry() + registry.registerSync(makePlugin({ name: 'dup' })) + + expect(() => registry.registerSync(makePlugin({ name: 'dup' }))).toThrow( + "Plugin 'dup' is already registered" + ) + }) + + it('should validate plugin structure', () => { + const registry = new PluginRegistry() + + expect(() => registry.registerSync({ name: '' } as FluxStack.Plugin)).toThrow( + 'Plugin must have a valid name property' + ) + }) + + it('should respect priority ordering', () => { + const registry = new PluginRegistry() + const low = makePlugin({ name: 'low', priority: 'low' }) + const high = makePlugin({ name: 'high', priority: 'high' }) + + registry.registerSync(low) + registry.registerSync(high) + + const order = registry.getLoadOrder() + expect(order.indexOf('high')).toBeLessThan(order.indexOf('low')) + }) + }) + + describe('load order — complex scenarios', () => { + it('should resolve a transitive dependency chain (A → B → C)', () => { + const registry = new PluginRegistry() + // Register in reverse order to prove topological sort works + registry.registerSync(makePlugin({ name: 'C', dependencies: ['B'] })) + registry.registerSync(makePlugin({ name: 'A' })) + registry.registerSync(makePlugin({ name: 'B', dependencies: ['A'] })) + + const order = registry.getLoadOrder() + expect(order.indexOf('A')).toBeLessThan(order.indexOf('B')) + expect(order.indexOf('B')).toBeLessThan(order.indexOf('C')) + }) + + it('should resolve a diamond dependency (D depends on B and C, both depend on A)', () => { + const registry = new PluginRegistry() + registry.registerSync(makePlugin({ name: 'D', dependencies: ['B', 'C'] })) + registry.registerSync(makePlugin({ name: 'B', dependencies: ['A'] })) + registry.registerSync(makePlugin({ name: 'C', dependencies: ['A'] })) + registry.registerSync(makePlugin({ name: 'A' })) + + const order = registry.getLoadOrder() + // A must come before B and C; B and C must come before D + expect(order.indexOf('A')).toBeLessThan(order.indexOf('B')) + expect(order.indexOf('A')).toBeLessThan(order.indexOf('C')) + expect(order.indexOf('B')).toBeLessThan(order.indexOf('D')) + expect(order.indexOf('C')).toBeLessThan(order.indexOf('D')) + }) + + it('should order all priority levels correctly (highest > high > normal > low > lowest)', () => { + const registry = new PluginRegistry() + registry.registerSync(makePlugin({ name: 'p-low', priority: 'low' })) + registry.registerSync(makePlugin({ name: 'p-highest', priority: 'highest' })) + registry.registerSync(makePlugin({ name: 'p-normal' })) // default = normal + registry.registerSync(makePlugin({ name: 'p-lowest', priority: 'lowest' })) + registry.registerSync(makePlugin({ name: 'p-high', priority: 'high' })) + + const order = registry.getLoadOrder() + expect(order).toEqual(['p-highest', 'p-high', 'p-normal', 'p-low', 'p-lowest']) + }) + + it('should let dependency constraint override priority', () => { + const registry = new PluginRegistry() + // dep-low has low priority but needs-low depends on it + registry.registerSync(makePlugin({ name: 'dep-low', priority: 'low' })) + registry.registerSync(makePlugin({ name: 'needs-low', priority: 'highest', dependencies: ['dep-low'] })) + + const order = registry.getLoadOrder() + expect(order.indexOf('dep-low')).toBeLessThan(order.indexOf('needs-low')) + }) + + it('should detect circular dependencies', () => { + const registry = new PluginRegistry() + registry.registerSync(makePlugin({ name: 'x', dependencies: ['y'] })) + + expect(() => { + registry.registerSync(makePlugin({ name: 'y', dependencies: ['x'] })) + }).toThrow(/[Cc]ircular dependency/) + }) + + it('should detect three-way circular dependency', () => { + const registry = new PluginRegistry() + registry.registerSync(makePlugin({ name: 'a1', dependencies: ['c1'] })) + registry.registerSync(makePlugin({ name: 'b1', dependencies: ['a1'] })) + + expect(() => { + registry.registerSync(makePlugin({ name: 'c1', dependencies: ['b1'] })) + }).toThrow(/[Cc]ircular dependency/) + }) + + it('should handle numeric priority values', () => { + const registry = new PluginRegistry() + registry.registerSync(makePlugin({ name: 'p100', priority: 100 })) + registry.registerSync(makePlugin({ name: 'p900', priority: 900 })) + registry.registerSync(makePlugin({ name: 'p500', priority: 500 })) + + const order = registry.getLoadOrder() + expect(order).toEqual(['p900', 'p500', 'p100']) + }) + + it('should handle mixed priority with multiple dependency levels', () => { + const registry = new PluginRegistry() + // Level 0 (no deps): core-high (high), core-low (low) + // Level 1 (depends on core-*): feature (depends on core-low) + registry.registerSync(makePlugin({ name: 'core-high', priority: 'high' })) + registry.registerSync(makePlugin({ name: 'core-low', priority: 'low' })) + registry.registerSync(makePlugin({ name: 'feature', dependencies: ['core-low'], priority: 'highest' })) + + const order = registry.getLoadOrder() + // Level 0: core-high before core-low (by priority) + expect(order.indexOf('core-high')).toBeLessThan(order.indexOf('core-low')) + // Level 1: feature after core-low (by dependency) + expect(order.indexOf('core-low')).toBeLessThan(order.indexOf('feature')) + }) + + it('should handle plugins with no dependencies among many', () => { + const registry = new PluginRegistry() + for (let i = 0; i < 10; i++) { + registry.registerSync(makePlugin({ name: `standalone-${i}` })) + } + + const order = registry.getLoadOrder() + expect(order.length).toBe(10) + // All should be present + for (let i = 0; i < 10; i++) { + expect(order).toContain(`standalone-${i}`) + } + }) + }) + + describe('refreshLoadOrder()', () => { + it('should recompute load order', () => { + const registry = new PluginRegistry() + registry.registerSync(makePlugin({ name: 'a' })) + registry.registerSync(makePlugin({ name: 'b' })) + + // Should not throw + registry.refreshLoadOrder() + + const order = registry.getLoadOrder() + expect(order).toContain('a') + expect(order).toContain('b') + }) + + it('should fall back to insertion-order on failure', () => { + const registry = new PluginRegistry() + registry.registerSync(makePlugin({ name: 'x' })) + registry.registerSync(makePlugin({ name: 'y' })) + + // refreshLoadOrder should always succeed (falls back gracefully) + registry.refreshLoadOrder() + + const order = registry.getLoadOrder() + expect(order.length).toBe(2) + }) + }) + + describe('getPluginsMap()', () => { + it('should return a map of all registered plugins', () => { + const registry = new PluginRegistry() + const a = makePlugin({ name: 'a' }) + const b = makePlugin({ name: 'b' }) + + registry.registerSync(a) + registry.registerSync(b) + + const map = registry.getPluginsMap() + expect(map.size).toBe(2) + expect(map.get('a')).toBe(a) + expect(map.get('b')).toBe(b) + }) + + it('should return an empty map when no plugins registered', () => { + const registry = new PluginRegistry() + const map = registry.getPluginsMap() + expect(map.size).toBe(0) + }) + }) + + describe('integration with FluxStackFramework.use()', () => { + it('should work end-to-end through framework use() and start()', async () => { + // This test imports the framework to ensure registerSync is wired correctly + const { FluxStackFramework } = await import('@core/framework/server') + + const hookOrder: string[] = [] + const framework = new FluxStackFramework({ + server: { + port: 0, + host: 'localhost', + apiPrefix: '/api', + cors: { origins: ['*'], methods: ['GET'], headers: ['Content-Type'], credentials: false, maxAge: 86400 }, + middleware: [] + } + }) + + framework.use({ + name: 'e2e-sync', + setup: () => { hookOrder.push('setup') }, + onBeforeServerStart: () => { hookOrder.push('beforeStart') }, + onServerStart: () => { hookOrder.push('start') }, + onAfterServerStart: () => { hookOrder.push('afterStart') } + }) + + await framework.start() + + expect(hookOrder).toEqual(['setup', 'beforeStart', 'start', 'afterStart']) + }) + }) +})