fix(tegg): address module plugin review feedback

Author: gxkl

tegg/core/aop-runtime/test/aop-runtime.test.ts

@@ -3,9 +3,9 @@ import path from 'node:path';
 import { mock } from 'node:test';
 
 import { CrosscutAdviceFactory } from '@eggjs/aop-decorator';
-import { EggPrototypeLifecycleUtil, LoadUnitFactory, LoadUnitLifecycleUtil } from '@eggjs/metadata';
-import { CoreTestHelper, EggTestContext } from '@eggjs/module-test-util';
-import { EggObjectLifecycleUtil, LoadUnitInstanceFactory } from '@eggjs/tegg-runtime';
+import { EggPrototypeLifecycleUtil, LoadUnitLifecycleUtil } from '@eggjs/metadata';
+import { CoreTestHelper, EggTestContext, LoaderUtil } from '@eggjs/module-test-util';
+import { EggObjectLifecycleUtil } from '@eggjs/tegg-runtime';
 import type { LoadUnitInstance } from '@eggjs/tegg-types';
 import { describe, beforeEach, afterEach, it } from 'vitest';
 
@@ -64,10 +64,7 @@ describe('test/aop-runtime.test.ts', () => {
     });
 
     afterEach(async () => {
-      for (const module of modules) {
-        await LoadUnitFactory.destroyLoadUnit(module.loadUnit);
-        await LoadUnitInstanceFactory.destroyLoadUnitInstance(module);
-      }
+      await CoreTestHelper.destroyModules(modules);
       EggPrototypeLifecycleUtil.deleteLifecycle(eggPrototypeCrossCutHook);
       LoadUnitLifecycleUtil.deleteLifecycle(loadUnitAopHook);
       EggObjectLifecycleUtil.deleteLifecycle(eggObjectAopHook);
@@ -182,12 +179,23 @@ describe('test/aop-runtime.test.ts', () => {
       EggObjectLifecycleUtil.registerLifecycle(eggObjectAopHook);
     });
 
+    afterEach(() => {
+      EggPrototypeLifecycleUtil.deleteLifecycle(eggPrototypeCrossCutHook);
+      LoadUnitLifecycleUtil.deleteLifecycle(loadUnitAopHook);
+      EggObjectLifecycleUtil.deleteLifecycle(eggObjectAopHook);
+    });
+
     it('should throw', async () => {
-      await assert.rejects(async () => {
-        await CoreTestHelper.prepareModules([
-          path.join(__dirname, 'fixtures/modules/should_throw'),
-        ]);
-      }, /Aop Advice\(PointcutAdvice\) not found in loadUnits/);
+      const modulePath = path.join(__dirname, 'fixtures/modules/should_throw');
+      const { innerObjectLoadUnitInstance } = await LoaderUtil.buildGlobalGraph([modulePath]);
+      try {
+        await assert.rejects(
+          () => CoreTestHelper.getLoadUnitInstance(modulePath),
+          /Aop Advice\(PointcutAdvice\) not found in loadUnits/,
+        );
+      } finally {
+        await CoreTestHelper.destroyModules([innerObjectLoadUnitInstance]);
+      }
     });
   });
 
@@ -219,10 +227,7 @@ describe('test/aop-runtime.test.ts', () => {
     });
 
     afterEach(async () => {
-      for (const module of modules) {
-        await LoadUnitFactory.destroyLoadUnit(module.loadUnit);
-        await LoadUnitInstanceFactory.destroyLoadUnitInstance(module);
-      }
+      await CoreTestHelper.destroyModules(modules);
       EggPrototypeLifecycleUtil.deleteLifecycle(eggPrototypeCrossCutHook);
       LoadUnitLifecycleUtil.deleteLifecycle(loadUnitAopHook);
       EggObjectLifecycleUtil.deleteLifecycle(eggObjectAopHook);

tegg/core/core-decorator/src/decorator/EggLifecycleProto.ts

@@ -26,7 +26,7 @@ export function EggLifecycleProto(params: CommonEggLifecycleProtoParams): Protot
 type EggLifecycleProtoDecoratorFactory = (params?: EggLifecycleProtoParams) => PrototypeDecorator;
 
 const createLifecycleProto = (type: EggLifecycleType): EggLifecycleProtoDecoratorFactory => {
-  return (params?: EggLifecycleProtoParams) => EggLifecycleProto({ type, ...params });
+  return (params?: EggLifecycleProtoParams) => EggLifecycleProto({ ...params, type });
 };
 
 export const LoadUnitLifecycleProto: EggLifecycleProtoDecoratorFactory = createLifecycleProto('LoadUnit');

tegg/core/core-decorator/test/inner-object-decorators.test.ts

@@ -123,6 +123,15 @@ describe('core/core-decorator/test/inner-object-decorators.test.ts', () => {
       }, /EggLifecycle decorator should have type property/);
     });
 
+    it('should not let factory params override the fixed lifecycle type', () => {
+      class FixedLoadUnitLifecycle {}
+      LoadUnitLifecycleProto({ type: 'EggObject' } as any)(FixedLoadUnitLifecycle);
+
+      assert.deepEqual(PrototypeUtil.getEggLifecyclePrototypeMetadata(FixedLoadUnitLifecycle), {
+        type: 'LoadUnit',
+      });
+    });
+
     it('should return undefined metadata for non lifecycle proto', () => {
       assert.equal(PrototypeUtil.getEggLifecyclePrototypeMetadata(Router), undefined);
     });

tegg/core/dynamic-inject-runtime/test/index.test.ts

@@ -1,9 +1,8 @@
 import assert from 'node:assert/strict';
 import path from 'node:path';
 
-import { LoadUnitFactory } from '@eggjs/metadata';
 import { EggTestContext, CoreTestHelper } from '@eggjs/module-test-util';
-import { type LoadUnitInstance, LoadUnitInstanceFactory } from '@eggjs/tegg-runtime';
+import { type LoadUnitInstance } from '@eggjs/tegg-runtime';
 import { describe, it, beforeEach, afterEach } from 'vitest';
 
 import { HelloService } from './fixtures/modules/dynamic-inject-module/HelloService.js';
@@ -18,10 +17,7 @@ describe('test/dynamic-inject-runtime.test.ts', () => {
   });
 
   afterEach(async () => {
-    for (const module of modules) {
-      await LoadUnitFactory.destroyLoadUnit(module.loadUnit);
-      await LoadUnitInstanceFactory.destroyLoadUnitInstance(module);
-    }
+    await CoreTestHelper.destroyModules(modules);
   });
 
   it('should work', async () => {

tegg/core/eventbus-runtime/test/EventBus.test.ts

@@ -4,10 +4,10 @@ import { mock } from 'node:test';
 
 import { PrototypeUtil } from '@eggjs/core-decorator';
 import { EventInfoUtil, CORK_ID } from '@eggjs/eventbus-decorator';
-import { type EggPrototype, LoadUnitFactory } from '@eggjs/metadata';
+import { type EggPrototype } from '@eggjs/metadata';
 import { CoreTestHelper, EggTestContext } from '@eggjs/module-test-util';
 import { TimerUtil } from '@eggjs/tegg-common-util';
-import { type LoadUnitInstance, LoadUnitInstanceFactory } from '@eggjs/tegg-runtime';
+import { type LoadUnitInstance } from '@eggjs/tegg-runtime';
 import { describe, it, beforeEach, afterEach } from 'vitest';
 
 import { EventContextFactory, EventHandlerFactory, SingletonEventBus } from '../src/index.ts';
@@ -26,10 +26,7 @@ describe('test/EventBus.test.ts', () => {
   });
 
   afterEach(async () => {
-    for (const module of modules) {
-      await LoadUnitFactory.destroyLoadUnit(module.loadUnit);
-      await LoadUnitInstanceFactory.destroyLoadUnitInstance(module);
-    }
+    await CoreTestHelper.destroyModules(modules);
     mock.reset();
   });
 

tegg/core/loader/src/LoaderFactory.ts

@@ -125,7 +125,7 @@ export class LoaderFactory {
       for (const clazz of clazzList) {
         // Inner object protos are also egg prototypes, so this branch must come first.
         if (PrototypeUtil.isEggInnerObject(clazz)) {
-          res.innerObjectClazzList.push(clazz);
+          res.innerObjectClazzList!.push(clazz);
         } else if (PrototypeUtil.isEggPrototype(clazz)) {
           res.clazzList.push(clazz);
         } else if (PrototypeUtil.isEggMultiInstancePrototype(clazz)) {

tegg/core/loader/test/LoaderInnerObject.test.ts

@@ -19,7 +19,7 @@ describe('core/loader/test/LoaderInnerObject.test.ts', () => {
   it('should divert inner object clazz to innerObjectClazzList', async () => {
     const [descriptor] = await LoaderFactory.loadApp([moduleRef]);
 
-    const innerNames = descriptor.innerObjectClazzList.map((t) => t.name).sort();
+    const innerNames = descriptor.innerObjectClazzList!.map((t) => t.name).sort();
     assert.deepEqual(innerNames, ['ControllerHook', 'FetchRouter']);
 
     // Inner object classes must NOT stay in clazzList.

tegg/core/metadata/src/model/ModuleDescriptor.ts

@@ -12,7 +12,7 @@ export interface ModuleDescriptor {
   optional?: boolean;
   clazzList: EggProtoImplClass[];
   multiInstanceClazzList: EggProtoImplClass[];
-  innerObjectClazzList: EggProtoImplClass[];
+  innerObjectClazzList?: EggProtoImplClass[];
   protos: ProtoDescriptor[];
 }
 
@@ -39,7 +39,7 @@ export class ModuleDescriptorDumper {
           return ModuleDescriptorDumper.stringifyClazz(t, moduleDescriptor);
         })
         .join(',')}],` +
-      `"innerObjectClazzList": [${moduleDescriptor.innerObjectClazzList
+      `"innerObjectClazzList": [${(moduleDescriptor.innerObjectClazzList ?? [])
         .map((t) => {
           return ModuleDescriptorDumper.stringifyClazz(t, moduleDescriptor);
         })
@@ -86,7 +86,7 @@ export class ModuleDescriptorDumper {
     for (const clazz of desc.multiInstanceClazzList) addClazz(clazz);
     // Inner object / lifecycle proto classes are diverted out of clazzList, but
     // their files must still be recorded so bundle mode re-imports them.
-    for (const clazz of desc.innerObjectClazzList) addClazz(clazz);
+    for (const clazz of desc.innerObjectClazzList ?? []) addClazz(clazz);
     return Array.from(fileSet);
   }
 
@@ -98,6 +98,7 @@ export class ModuleDescriptorDumper {
     const tmpPath = path.join(tmpDir, path.basename(dumpPath));
     try {
       await fs.writeFile(tmpPath, ModuleDescriptorDumper.stringifyDescriptor(desc));
+      await fs.rm(dumpPath, { force: true });
       await fs.rename(tmpPath, dumpPath);
     } finally {
       await fs.rm(tmpDir, { recursive: true, force: true });

tegg/core/metadata/src/model/graph/GlobalGraph.ts

@@ -60,6 +60,7 @@ export class GlobalGraph {
   moduleProtoDescriptorMap: Map<string, ProtoDescriptor[]>;
   strict: boolean;
   private buildHooks: GlobalGraphBuildHook[];
+  #buildState: 'created' | 'building' | 'built' = 'created';
   /** Lazily built proto-name index for dependency resolution; invalidated on vertex changes. */
   #protoNameIndex: ProtoNameIndex | null = null;
 
@@ -94,31 +95,42 @@ export class GlobalGraph {
   }
 
   registerBuildHook(hook: GlobalGraphBuildHook): void {
+    if (this.#buildState !== 'created') {
+      throw new Error(`cannot register global graph build hook after build has started (state: ${this.#buildState})`);
+    }
     this.buildHooks.push(hook);
   }
 
   addModuleNode(moduleNode: GlobalModuleNode): void {
     if (!this.moduleGraph.addVertex(new GraphNode<GlobalModuleNode, ModuleDependencyMeta>(moduleNode))) {
       throw new Error(`duplicate module: ${moduleNode}`);
     }
+    this.#protoNameIndex = null;
     for (const protoNode of moduleNode.protos) {
       if (!this.protoGraph.addVertex(protoNode)) {
         throw new Error(`duplicate proto: ${protoNode.val}`);
       }
     }
-    this.#protoNameIndex = null;
   }
 
   build(): void {
-    for (const moduleNode of this.moduleGraph.nodes.values()) {
-      for (const protoNode of moduleNode.val.protos) {
-        for (const injectObj of protoNode.val.proto.injectObjects) {
-          this.buildInjectEdge(moduleNode, protoNode, injectObj);
+    if (this.#buildState !== 'created') {
+      throw new Error(`global graph can only be built once (state: ${this.#buildState})`);
+    }
+    this.#buildState = 'building';
+    try {
+      for (const moduleNode of this.moduleGraph.nodes.values()) {
+        for (const protoNode of moduleNode.val.protos) {
+          for (const injectObj of protoNode.val.proto.injectObjects) {
+            this.buildInjectEdge(moduleNode, protoNode, injectObj);
+          }
         }
       }
-    }
-    for (const buildHook of this.buildHooks) {
-      buildHook(this);
+      for (const buildHook of this.buildHooks) {
+        buildHook(this);
+      }
+    } finally {
+      this.#buildState = 'built';
     }
   }
 

tegg/core/metadata/test/GlobalGraph.test.ts

@@ -184,4 +184,65 @@ describe('test/LoadUnit/GlobalGraph.test.ts', () => {
       ['logger', 'bar', 'constructorBase', 'fooConstructor', 'fooConstructorLogger'],
     );
   });
+
+  it('should reject late build hooks and repeated builds', () => {
+    const graph = new GlobalGraph();
+    let hookCalls = 0;
+    graph.registerBuildHook(() => hookCalls++);
+
+    graph.build();
+
+    assert.equal(hookCalls, 1);
+    assert.throws(
+      () => graph.registerBuildHook(() => {}),
+      /cannot register global graph build hook after build has started/,
+    );
+    assert.throws(() => graph.build(), /global graph can only be built once/);
+  });
+
+  it('should reject build hook registration while build hooks are running', () => {
+    const graph = new GlobalGraph();
+    let nestedHookCalled = false;
+    graph.registerBuildHook(() => {
+      graph.registerBuildHook(() => {
+        nestedHookCalled = true;
+      });
+    });
+
+    assert.throws(
+      () => graph.build(),
+      /cannot register global graph build hook after build has started \(state: building\)/,
+    );
+    assert.equal(nestedHookCalled, false);
+    assert.throws(() => graph.build(), /global graph can only be built once \(state: built\)/);
+  });
+
+  it('should invalidate the proto name index before a partial add failure', () => {
+    const graph = new GlobalGraph();
+    const consumer = createProtoDescriptor({ name: 'consumer' });
+    const existing = createProtoDescriptor({ name: 'existing' });
+    const initialNode = new GlobalModuleNode({ name: 'initial', unitPath: '/fixtures/initial', optional: false });
+    initialNode.addProto(consumer);
+    initialNode.addProto(existing);
+    graph.addModuleNode(initialNode);
+
+    assert(graph.findDependencyProtoNode(consumer, { refName: 'existing', objName: 'existing', qualifiers: [] }));
+
+    const addedBeforeFailure = createProtoDescriptor({ name: 'addedBeforeFailure', moduleName: 'partial' });
+    const duplicate = createProtoDescriptor({ name: 'existing', moduleName: 'partial' });
+    duplicate.instanceModuleName = existing.instanceModuleName;
+    duplicate.instanceDefineUnitPath = existing.instanceDefineUnitPath;
+    const partialNode = new GlobalModuleNode({ name: 'partial', unitPath: '/fixtures/partial', optional: false });
+    partialNode.addProto(addedBeforeFailure);
+    partialNode.addProto(duplicate);
+
+    assert.throws(() => graph.addModuleNode(partialNode), /duplicate proto/);
+    assert(
+      graph.findDependencyProtoNode(consumer, {
+        refName: 'addedBeforeFailure',
+        objName: 'addedBeforeFailure',
+        qualifiers: [],
+      }),
+    );
+  });
 });