Skip to content

chore: use GlobalGraph to replace AppGraph/ModuleGraph #242

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 7, 2024
Merged

chore: use GlobalGraph to replace AppGraph/ModuleGraph #242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,3 +47,4 @@ plugin/tegg/test/fixtures/apps/**/*.js
!plugin/tegg/test/fixtures/**/node_modules
!plugin/config/test/fixtures/**/node_modules
.node
.egg
87 changes: 55 additions & 32 deletions core/common-util/src/Graph.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,15 @@ import type { GraphNodeObj } from '@eggjs/tegg-types';

const inspect = Symbol.for('nodejs.util.inspect.custom');

export class GraphNode<T extends GraphNodeObj> {
export interface EdgeMeta {
equal(meta: EdgeMeta): boolean;
toString(): string;
}

export class GraphNode<T extends GraphNodeObj, M extends EdgeMeta = EdgeMeta> {
val: T;
toNodeMap: Map<string, GraphNode<T>> = new Map();
fromNodeMap: Map<string, GraphNode<T>> = new Map();
toNodeMap: Map<string, {node: GraphNode<T, M>, meta?: M}> = new Map();
fromNodeMap: Map<string, {node: GraphNode<T, M>, meta?: M}> = new Map();
Comment on lines +12 to +13
Copy link

@coderabbitai coderabbitai bot Oct 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Consider supporting multiple edges between the same nodes with different metadata.

Currently, toNodeMap and fromNodeMap in the GraphNode class use node.id as the key. This design prevents adding multiple edges between the same pair of nodes when the meta data differs. If your intention is to support multiple edges with different metadata between the same nodes, consider modifying the data structures to accommodate this. One approach is to change the maps to use a composite key of node.id and a metadata identifier, or to store arrays of edges.


constructor(val: T) {
this.val = val;
Expand All @@ -15,19 +20,19 @@ export class GraphNode<T extends GraphNodeObj> {
return this.val.id;
}

addToVertex(node: GraphNode<T>) {
addToVertex(node: GraphNode<T, M>, meta?: M) {
if (this.toNodeMap.has(node.id)) {
return false;
}
this.toNodeMap.set(node.id, node);
this.toNodeMap.set(node.id, { node, meta });
return true;
}

addFromVertex(node: GraphNode<T>) {
addFromVertex(node: GraphNode<T, M>, meta?: M) {
if (this.fromNodeMap.has(node.id)) {
return false;
}
this.fromNodeMap.set(node.id, node);
this.fromNodeMap.set(node.id, { node, meta });
return true;
}

Expand All @@ -48,69 +53,87 @@ export class GraphNode<T extends GraphNodeObj> {
}
}

export class GraphPath<T extends GraphNodeObj> {
export class GraphPath<T extends GraphNodeObj, M extends EdgeMeta = EdgeMeta> {
nodeIdMap: Map<string, number> = new Map();
nodes: Array<GraphNode<T>> = [];
nodes: Array<{ node: GraphNode<T, M>, meta?: M }> = [];

pushVertex(node: GraphNode<T>): boolean {
pushVertex(node: GraphNode<T, M>, meta?: M): boolean {
const val = this.nodeIdMap.get(node.id) || 0;
this.nodeIdMap.set(node.id, val + 1);
this.nodes.push(node);
this.nodes.push({ node, meta });
return val === 0;
}

popVertex() {
const node = this.nodes.pop();
if (node) {
const val = this.nodeIdMap.get(node.id)!;
this.nodeIdMap.set(node.id, val - 1);
const nodeHandler = this.nodes.pop();
if (nodeHandler) {
const val = this.nodeIdMap.get(nodeHandler.node.id)!;
this.nodeIdMap.set(nodeHandler.node.id, val - 1);
}
}

toString() {
const res = this.nodes.reduce((p, c) => {
p.push(c.val.toString());
let msg = '';
if (c.meta) {
msg += ` ${c.meta.toString()} -> `;
} else if (p.length) {
msg += ' -> ';
}
msg += c.node.val.toString();
p.push(msg);
return p;
}, new Array<string>());
return res.join(' -> ');
return res.join('');
}

[inspect]() {
return this.toString();
}
}

export class Graph<T extends GraphNodeObj> {
nodes: Map<string, GraphNode<T>> = new Map();
export class Graph<T extends GraphNodeObj, M extends EdgeMeta = EdgeMeta> {
nodes: Map<string, GraphNode<T, M>> = new Map();

addVertex(node: GraphNode<T>): boolean {
addVertex(node: GraphNode<T, M>): boolean {
if (this.nodes.has(node.id)) {
return false;
}
this.nodes.set(node.id, node);
return true;
}

addEdge(from: GraphNode<T>, to: GraphNode<T>): boolean {
to.addFromVertex(from);
return from.addToVertex(to);
addEdge(from: GraphNode<T, M>, to: GraphNode<T, M>, meta?: M): boolean {
to.addFromVertex(from, meta);
return from.addToVertex(to, meta);
}

findToNode(id: string, meta: M): GraphNode<T, M> | undefined {
const node = this.nodes.get(id);
if (!node) return undefined;
for (const { node: toNode, meta: edgeMeta } of node.toNodeMap.values()) {
if (edgeMeta && meta.equal(edgeMeta)) {
return toNode;
}
}
return undefined;
}

appendVertexToPath(node: GraphNode<T>, accessPath: GraphPath<T>): boolean {
if (!accessPath.pushVertex(node)) {
appendVertexToPath(node: GraphNode<T, M>, accessPath: GraphPath<T, M>, meta?: M): boolean {
if (!accessPath.pushVertex(node, meta)) {
return false;
}
for (const toNode of node.toNodeMap.values()) {
if (!this.appendVertexToPath(toNode, accessPath)) {
if (!this.appendVertexToPath(toNode.node, accessPath, toNode.meta)) {
return false;
Comment on lines +122 to 128
Copy link

@coderabbitai coderabbitai bot Oct 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Potential stack overflow due to deep recursion in appendVertexToPath.

The appendVertexToPath method uses recursion to traverse the graph:

for (const toNode of node.toNodeMap.values()) {
  if (!this.appendVertexToPath(toNode.node, accessPath, toNode.meta)) {
    return false;
  }
}

In graphs with a large depth or in the presence of cycles (even though cycles are handled), this could lead to a stack overflow. Consider refactoring this recursive approach into an iterative one using a stack or queue to manage traversal, which can improve performance and prevent potential stack issues.

}
}
accessPath.popVertex();
return true;
}

loopPath(): GraphPath<T> | undefined {
const accessPath = new GraphPath<T>();
loopPath(): GraphPath<T, M> | undefined {
const accessPath = new GraphPath<T, M>();
const nodes = Array.from(this.nodes.values());
for (const node of nodes) {
if (!this.appendVertexToPath(node, accessPath)) {
Expand All @@ -120,7 +143,7 @@ export class Graph<T extends GraphNodeObj> {
return;
}

accessNode(node: GraphNode<T>, nodes: Array<GraphNode<T>>, accessed: boolean[], res: Array<GraphNode<T>>) {
accessNode(node: GraphNode<T, M>, nodes: Array<GraphNode<T, M>>, accessed: boolean[], res: Array<GraphNode<T, M>>) {
const index = nodes.indexOf(node);
if (accessed[index]) {
return;
Expand All @@ -131,7 +154,7 @@ export class Graph<T extends GraphNodeObj> {
return;
}
for (const toNode of node.toNodeMap.values()) {
this.accessNode(toNode, nodes, accessed, res);
this.accessNode(toNode.node, nodes, accessed, res);
}
accessed[nodes.indexOf(node)] = true;
res.push(node);
Expand All @@ -145,8 +168,8 @@ export class Graph<T extends GraphNodeObj> {
// notice:
// 1. sort result is not stable
// 2. graph with loop can not be sort
sort(): Array<GraphNode<T>> {
const res: Array<GraphNode<T>> = [];
sort(): Array<GraphNode<T, M>> {
const res: Array<GraphNode<T, M>> = [];
const nodes = Array.from(this.nodes.values());
const accessed: boolean[] = [];
for (let i = 0; i < nodes.length; ++i) {
Expand Down
47 changes: 46 additions & 1 deletion core/core-decorator/src/util/PrototypeUtil.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ import {
InjectConstructorInfo,
InjectObjectInfo,
InjectType, LoadUnitNameQualifierAttribute,
MultiInstancePrototypeGetObjectsContext, QualifierAttribute,
MultiInstancePrototypeGetObjectsContext,
MultiInstanceType,
QualifierAttribute,
} from '@eggjs/tegg-types';
import { MetadataUtil } from './MetadataUtil';

Expand Down Expand Up @@ -57,6 +59,21 @@ export class PrototypeUtil {
return MetadataUtil.getBooleanMetaData(PrototypeUtil.IS_EGG_OBJECT_MULTI_INSTANCE_PROTOTYPE, clazz);
}

/**
* Get the type of the egg multi-instance prototype.
* @param {Function} clazz -
*/
static getEggMultiInstancePrototypeType(clazz: EggProtoImplClass): MultiInstanceType | undefined {
if (!PrototypeUtil.isEggMultiInstancePrototype(clazz)) {
return;
}
const metadata = MetadataUtil.getMetaData<EggMultiInstancePrototypeInfo>(PrototypeUtil.MULTI_INSTANCE_PROTOTYPE_STATIC_PROPERTY, clazz);
if (metadata) {
return MultiInstanceType.STATIC;
}
return MultiInstanceType.DYNAMIC;
}

/**
* set class file path
* @param {Function} clazz -
Expand Down Expand Up @@ -129,6 +146,33 @@ export class PrototypeUtil {
MetadataUtil.defineMetaData(PrototypeUtil.MULTI_INSTANCE_PROTOTYPE_CALLBACK_PROPERTY, property, clazz);
}

/**
* Get instance property of Static multi-instance prototype.
* @param {EggProtoImplClass} clazz -
*/
static getStaticMultiInstanceProperty(clazz: EggProtoImplClass): EggMultiInstancePrototypeInfo | undefined {
const metadata = MetadataUtil.getMetaData<EggMultiInstancePrototypeInfo>(PrototypeUtil.MULTI_INSTANCE_PROTOTYPE_STATIC_PROPERTY, clazz);
if (metadata) {
return metadata;
}
}

/**
* Get instance property of Dynamic multi-instance prototype.
* @param {EggProtoImplClass} clazz -
* @param {MultiInstancePrototypeGetObjectsContext} ctx -
*/
static getDynamicMultiInstanceProperty(clazz: EggProtoImplClass, ctx: MultiInstancePrototypeGetObjectsContext): EggMultiInstancePrototypeInfo | undefined {
const callBackMetadata = MetadataUtil.getMetaData<EggMultiInstanceCallbackPrototypeInfo>(PrototypeUtil.MULTI_INSTANCE_PROTOTYPE_CALLBACK_PROPERTY, clazz);
if (callBackMetadata) {
const objects = callBackMetadata.getObjects(ctx);
return {
...callBackMetadata,
objects,
};
}
}

/**
* get class property
* @param {EggProtoImplClass} clazz -
Expand All @@ -142,6 +186,7 @@ export class PrototypeUtil {
const callBackMetadata = MetadataUtil.getMetaData<EggMultiInstanceCallbackPrototypeInfo>(PrototypeUtil.MULTI_INSTANCE_PROTOTYPE_CALLBACK_PROPERTY, clazz);
if (callBackMetadata) {
const objects = callBackMetadata.getObjects(ctx);
// TODO delete in next major version, default qualifier be added in ProtoDescriptorHelper.addDefaultQualifier
const defaultQualifier = [{
attribute: InitTypeQualifierAttribute,
value: callBackMetadata.initType,
Expand Down
14 changes: 14 additions & 0 deletions core/core-decorator/src/util/QualifierUtil.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,4 +66,18 @@ export class QualifierUtil {
const qualifiers = properQualifiers?.get(property);
return qualifiers?.get(attribute);
}

static matchQualifiers(clazzQualifiers: QualifierInfo[], requestQualifiers: QualifierInfo[]): boolean {
for (const request of requestQualifiers) {
if (!clazzQualifiers.find(t => t.attribute === request.attribute && t.value === request.value)) {
return false;
}
}
return true;
}

static equalQualifiers(clazzQualifiers: QualifierInfo[], requestQualifiers: QualifierInfo[]): boolean {
if (clazzQualifiers.length !== requestQualifiers.length) return false;
return QualifierUtil.matchQualifiers(clazzQualifiers, requestQualifiers);
}
}
30 changes: 29 additions & 1 deletion core/loader/src/LoaderFactory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import type { EggLoadUnitTypeLike, Loader } from '@eggjs/tegg-types';
import { EggLoadUnitType, EggLoadUnitTypeLike, EggProtoImplClass, Loader, ModuleReference } from '@eggjs/tegg-types';
import { ModuleDescriptor } from '@eggjs/tegg-metadata';
import { PrototypeUtil } from '@eggjs/core-decorator';

export type LoaderCreator = (unitPath: string) => Loader;

Expand All @@ -16,4 +18,30 @@ export class LoaderFactory {
static registerLoader(type: EggLoadUnitTypeLike, creator: LoaderCreator) {
this.loaderCreatorMap.set(type, creator);
}

static loadApp(moduleReferences: readonly ModuleReference[]): ModuleDescriptor[] {
const result: ModuleDescriptor[] = [];
const multiInstanceClazzList: EggProtoImplClass[] = [];
Copy link

@coderabbitai coderabbitai bot Oct 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Initialize multiInstanceClazzList inside the loop to avoid shared state issues.

Currently, multiInstanceClazzList is defined outside the loop and shared across all modules, which may lead to unexpected behavior if modules modify this list.

Apply this diff to fix the issue:

- const multiInstanceClazzList: EggProtoImplClass[] = [];
for (const moduleReference of moduleReferences) {
+  const multiInstanceClazzList: EggProtoImplClass[] = [];

Committable suggestion was skipped due to low confidence.

for (const moduleReference of moduleReferences) {
const loader = LoaderFactory.createLoader(moduleReference.path, moduleReference.loaderType || EggLoadUnitType.MODULE);
const res: ModuleDescriptor = {
name: moduleReference.name,
unitPath: moduleReference.path,
clazzList: [],
protos: [],
multiInstanceClazzList,
optional: moduleReference.optional,
};
result.push(res);
const clazzList = loader.load();
for (const clazz of clazzList) {
if (PrototypeUtil.isEggPrototype(clazz)) {
res.clazzList.push(clazz);
} else if (PrototypeUtil.isEggMultiInstancePrototype(clazz)) {
res.multiInstanceClazzList.push(clazz);
}
}
Comment on lines +38 to +43
Copy link

@coderabbitai coderabbitai bot Oct 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle classes that are neither single-instance nor multi-instance prototypes.

The current implementation only checks for isEggPrototype and isEggMultiInstancePrototype. It's possible there are classes that do not satisfy either condition, which will be silently ignored.

Consider adding an else clause to handle or log unexpected classes:

if (PrototypeUtil.isEggPrototype(clazz)) {
  res.clazzList.push(clazz);
} else if (PrototypeUtil.isEggMultiInstancePrototype(clazz)) {
  res.multiInstanceClazzList.push(clazz);
} else {
  console.warn(`Unhandled class type: ${clazz.name}`);
}

}
return result;
}
Comment on lines +22 to +46
Copy link

@coderabbitai coderabbitai bot Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Refactor the loadApp method to address potential issues and improve maintainability.

While the method implements the core functionality described in the PR objectives, there are several areas for improvement:

  1. Initialize multiInstanceClazzList inside the loop to avoid shared state issues.
  2. Add error handling around loader.load() to gracefully handle potential exceptions.
  3. Add JSDoc comments to improve documentation and maintainability.
  4. Consider handling classes that are neither single-instance nor multi-instance prototypes.

Here's a suggested refactor addressing these points:

/**
 * Load application modules and return their descriptors.
 * @param moduleReferences - An array of module references to load.
 * @returns An array of ModuleDescriptor objects representing loaded modules.
 */
static loadApp(moduleReferences: readonly ModuleReference[]): ModuleDescriptor[] {
  const result: ModuleDescriptor[] = [];
  for (const moduleReference of moduleReferences) {
    const multiInstanceClazzList: EggProtoImplClass[] = [];
    const loader = LoaderFactory.createLoader(moduleReference.path, moduleReference.loaderType || EggLoadUnitType.MODULE);
    const res: ModuleDescriptor = {
      name: moduleReference.name,
      unitPath: moduleReference.path,
      clazzList: [],
      protos: [],
      multiInstanceClazzList,
      optional: moduleReference.optional,
    };
    result.push(res);
    try {
      const clazzList = loader.load();
      for (const clazz of clazzList) {
        if (PrototypeUtil.isEggPrototype(clazz)) {
          res.clazzList.push(clazz);
        } else if (PrototypeUtil.isEggMultiInstancePrototype(clazz)) {
          res.multiInstanceClazzList.push(clazz);
        } else {
          console.warn(`Unhandled class type: ${clazz.name}`);
        }
      }
    } catch (error) {
      console.error(`Failed to load module at ${moduleReference.path}:`, error);
      // Decide whether to continue or throw the error based on your error handling strategy
    }
  }
  return result;
}

This refactored version addresses the identified issues and improves the overall robustness of the method.

}
9 changes: 9 additions & 0 deletions core/metadata/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,14 @@ export * from './src/util/ClassUtil';
export * from './src/impl/LoadUnitMultiInstanceProtoHook';
export * from './src/model/AppGraph';

export * from './src/model/graph/GlobalGraph';
export * from './src/model/graph/GlobalModuleNode';
export * from './src/model/graph/GlobalModuleNodeBuilder';
export * from './src/model/graph/ProtoNode';
export * from './src/model/graph/ProtoSelector';
export * from './src/model/ProtoDescriptor/AbstractProtoDescriptor';
export * from './src/model/ProtoDescriptor/ClassProtoDescriptor';
export * from './src/model/ModuleDescriptor';

import './src/impl/ModuleLoadUnit';
import './src/impl/EggPrototypeBuilder';
21 changes: 21 additions & 0 deletions core/metadata/src/factory/EggPrototypeCreatorFactory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import type {
EggPrototypeLifecycleContext,
} from '@eggjs/tegg-types';
import { EggPrototypeLifecycleUtil } from '../model/EggPrototype';
import { ClassProtoDescriptor } from '../model/ProtoDescriptor/ClassProtoDescriptor';

export class EggPrototypeCreatorFactory {
private static creatorMap = new Map<string, EggPrototypeCreator>();
Expand Down Expand Up @@ -81,4 +82,24 @@ export class EggPrototypeCreatorFactory {
return protos;

}

static async createProtoByDescriptor(protoDescriptor: ClassProtoDescriptor, loadUnit: LoadUnit): Promise<EggPrototype> {
const creator = this.getPrototypeCreator(protoDescriptor.protoImplType);
if (!creator) {
throw new Error(`not found proto creator for type: ${protoDescriptor.protoImplType}`);
}
const ctx: EggPrototypeLifecycleContext = {
clazz: protoDescriptor.clazz,
loadUnit,
prototypeInfo: protoDescriptor,
};
const proto = creator(ctx);
await EggPrototypeLifecycleUtil.objectPreCreate(ctx, proto);
if (proto.init) {
await proto.init(ctx);
}
await EggPrototypeLifecycleUtil.objectPostCreate(ctx, proto);
PrototypeUtil.setClazzProto(protoDescriptor.clazz, proto);
return proto;
}
}
4 changes: 1 addition & 3 deletions core/metadata/src/impl/EggPrototypeBuilder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export class EggPrototypeBuilder {
private injectObjects: Array<InjectObject | InjectConstructor> = [];
private loadUnit: LoadUnit;
private qualifiers: QualifierInfo[] = [];
private properQualifiers: Record<string, QualifierInfo[]> = {};
private properQualifiers: Record<PropertyKey, QualifierInfo[]> = {};
private className?: string;
private multiInstanceConstructorIndex?: number;
private multiInstanceConstructorAttributes?: QualifierAttribute[];
Expand All @@ -57,7 +57,6 @@ export class EggPrototypeBuilder {
...QualifierUtil.getProtoQualifiers(clazz),
...(ctx.prototypeInfo.qualifiers ?? []),
];
console.log('proto: ', ctx.prototypeInfo.properQualifiers);
builder.properQualifiers = ctx.prototypeInfo.properQualifiers ?? {};
builder.multiInstanceConstructorIndex = PrototypeUtil.getMultiInstanceConstructorIndex(clazz);
builder.multiInstanceConstructorAttributes = PrototypeUtil.getMultiInstanceConstructorAttributes(clazz);
Expand All @@ -67,7 +66,6 @@ export class EggPrototypeBuilder {
private tryFindDefaultPrototype(injectObject: InjectObject): EggPrototype {
const propertyQualifiers = QualifierUtil.getProperQualifiers(this.clazz, injectObject.refName);
const multiInstancePropertyQualifiers = this.properQualifiers[injectObject.refName as string] ?? [];
console.log('multi instance: ', this.properQualifiers, injectObject.refName);
return EggPrototypeFactory.instance.getPrototype(injectObject.objName, this.loadUnit, [
...propertyQualifiers,
...multiInstancePropertyQualifiers,
Expand Down
Loading
Loading