-
Notifications
You must be signed in to change notification settings - Fork 35
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.
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
feat: impl GlobalGraph build hook #246
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,6 @@ | ||
export * from './src/EggPrototypeCrossCutHook'; | ||
export * from './src/EggObjectAopHook'; | ||
export * from './src/LoadUnitAopHook'; | ||
|
||
export * from './src/CrossCutGraphHook'; | ||
export * from './src/PointCutGraphHook'; |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,63 @@ | ||||||||||||||||||||||||||||||||
import { AspectMetaBuilder, CrosscutInfo, CrosscutInfoUtil } from '@eggjs/aop-decorator'; | ||||||||||||||||||||||||||||||||
import { GraphNode } from '@eggjs/tegg-common-util'; | ||||||||||||||||||||||||||||||||
import { | ||||||||||||||||||||||||||||||||
ClassProtoDescriptor, | ||||||||||||||||||||||||||||||||
GlobalGraph, | ||||||||||||||||||||||||||||||||
ProtoDependencyMeta, | ||||||||||||||||||||||||||||||||
ProtoNode, | ||||||||||||||||||||||||||||||||
} from '@eggjs/tegg-metadata'; | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
export function crossCutGraphHook(globalGraph: GlobalGraph) { | ||||||||||||||||||||||||||||||||
for (const moduleNode of globalGraph.moduleGraph.nodes.values()) { | ||||||||||||||||||||||||||||||||
for (const crossCutProtoNode of moduleNode.val.protos) { | ||||||||||||||||||||||||||||||||
const protoNodes = findCrossCuttedClazz(globalGraph, crossCutProtoNode); | ||||||||||||||||||||||||||||||||
if (!protoNodes) continue; | ||||||||||||||||||||||||||||||||
for (const crossCuttedProtoNode of protoNodes) { | ||||||||||||||||||||||||||||||||
const crossCuttedModuleNode = globalGraph.findModuleNode(crossCuttedProtoNode.val.proto.instanceModuleName); | ||||||||||||||||||||||||||||||||
if (!crossCuttedModuleNode) continue; | ||||||||||||||||||||||||||||||||
globalGraph.addInject( | ||||||||||||||||||||||||||||||||
crossCuttedModuleNode, | ||||||||||||||||||||||||||||||||
crossCuttedProtoNode, | ||||||||||||||||||||||||||||||||
crossCutProtoNode, | ||||||||||||||||||||||||||||||||
crossCutProtoNode.val.proto.name); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
function findCrossCuttedClazz(globalGraph: GlobalGraph, protoNode: GraphNode<ProtoNode, ProtoDependencyMeta>) { | ||||||||||||||||||||||||||||||||
const proto = protoNode.val.proto; | ||||||||||||||||||||||||||||||||
if (!ClassProtoDescriptor.isClassProtoDescriptor(proto)) { | ||||||||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
if (!CrosscutInfoUtil.isCrosscutAdvice(proto.clazz)) { | ||||||||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||||||||
Comment on lines
+28
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure In the Apply this diff to ensure consistent return values: function findCrossCuttedClazz(globalGraph: GlobalGraph, protoNode: GraphNode<ProtoNode, ProtoDependencyMeta>) {
const proto = protoNode.val.proto;
if (!ClassProtoDescriptor.isClassProtoDescriptor(proto)) {
- return;
+ return [];
}
if (!CrosscutInfoUtil.isCrosscutAdvice(proto.clazz)) {
- return;
+ return [];
}
const crosscutInfoList = CrosscutInfoUtil.getCrosscutInfoList(proto.clazz); 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
const crosscutInfoList = CrosscutInfoUtil.getCrosscutInfoList(proto.clazz); | ||||||||||||||||||||||||||||||||
const result: GraphNode<ProtoNode, ProtoDependencyMeta>[] = []; | ||||||||||||||||||||||||||||||||
// eslint-disable-next-line no-labels | ||||||||||||||||||||||||||||||||
crosscut: for (const crosscutInfo of crosscutInfoList) { | ||||||||||||||||||||||||||||||||
for (const protoNode of globalGraph.protoGraph.nodes.values()) { | ||||||||||||||||||||||||||||||||
if (checkClazzMatchCrossCut(protoNode, crosscutInfo)) { | ||||||||||||||||||||||||||||||||
Comment on lines
+40
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Avoid Variable Shadowing for Clarity In the inner loop of Apply this diff to rename the loop variable: - for (const protoNode of globalGraph.protoGraph.nodes.values()) {
- if (checkClazzMatchCrossCut(protoNode, crosscutInfo)) {
+ for (const candidateProtoNode of globalGraph.protoGraph.nodes.values()) {
+ if (checkClazzMatchCrossCut(candidateProtoNode, crosscutInfo)) { Remember to update all usages of 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||
result.push(protoNode); | ||||||||||||||||||||||||||||||||
// eslint-disable-next-line no-labels | ||||||||||||||||||||||||||||||||
break crosscut; | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
Comment on lines
+38
to
+45
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Refactor to Eliminate Labeled Breaks The use of labels ( Here's how you could refactor the code: -function findCrossCuttedClazz(globalGraph: GlobalGraph, protoNode: GraphNode<ProtoNode, ProtoDependencyMeta>) {
// ... existing code ...
- // eslint-disable-next-line no-labels
- crosscut: for (const crosscutInfo of crosscutInfoList) {
- for (const candidateProtoNode of globalGraph.protoGraph.nodes.values()) {
- if (checkClazzMatchCrossCut(candidateProtoNode, crosscutInfo)) {
- result.push(candidateProtoNode);
- // eslint-disable-next-line no-labels
- break crosscut;
- }
- }
+ for (const crosscutInfo of crosscutInfoList) {
+ const matchedNodes = Array.from(globalGraph.protoGraph.nodes.values()).filter(candidateProtoNode =>
+ checkClazzMatchCrossCut(candidateProtoNode, crosscutInfo)
+ );
+ result.push(...matchedNodes);
}
return result;
} This refactoring removes the labeled break by collecting all matched nodes and avoids breaking out of nested loops.
|
||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
return result; | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
function checkClazzMatchCrossCut(protoNode: GraphNode<ProtoNode>, crosscutInfo: CrosscutInfo) { | ||||||||||||||||||||||||||||||||
const proto = protoNode.val.proto; | ||||||||||||||||||||||||||||||||
if (!ClassProtoDescriptor.isClassProtoDescriptor(proto)) { | ||||||||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
const allMethods = AspectMetaBuilder.getAllMethods(proto.clazz); | ||||||||||||||||||||||||||||||||
for (const method of allMethods) { | ||||||||||||||||||||||||||||||||
if (crosscutInfo.pointcutInfo.match(proto.clazz, method)) { | ||||||||||||||||||||||||||||||||
return true; | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
return false; | ||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,54 @@ | ||||||||||
import { AspectMetaBuilder, PointcutAdviceInfoUtil } from '@eggjs/aop-decorator'; | ||||||||||
import { PrototypeUtil, QualifierUtil } from '@eggjs/core-decorator'; | ||||||||||
import { GraphNode } from '@eggjs/tegg-common-util'; | ||||||||||
import { | ||||||||||
ClassProtoDescriptor, | ||||||||||
GlobalGraph, | ||||||||||
ProtoDependencyMeta, | ||||||||||
ProtoNode, | ||||||||||
} from '@eggjs/tegg-metadata'; | ||||||||||
import assert from 'node:assert'; | ||||||||||
|
||||||||||
export function pointCutGraphHook(globalGraph: GlobalGraph) { | ||||||||||
for (const moduleNode of globalGraph.moduleGraph.nodes.values()) { | ||||||||||
for (const pointCuttedProtoNode of moduleNode.val.protos) { | ||||||||||
const pointCutAdviceProtoList = findPointCutAdvice(globalGraph, pointCuttedProtoNode); | ||||||||||
if (!pointCutAdviceProtoList) continue; | ||||||||||
Comment on lines
+15
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Simplify condition by checking array length With Apply this diff: - if (!pointCutAdviceProtoList) continue;
+ if (pointCutAdviceProtoList.length === 0) continue; This makes the code more readable and avoids potential issues with falsy values other than 📝 Committable suggestion
Suggested change
|
||||||||||
for (const pointCutAdviceProto of pointCutAdviceProtoList) { | ||||||||||
globalGraph.addInject( | ||||||||||
moduleNode, | ||||||||||
pointCuttedProtoNode, | ||||||||||
pointCutAdviceProto, | ||||||||||
pointCutAdviceProto.val.proto.name); | ||||||||||
} | ||||||||||
} | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
function findPointCutAdvice(globalGraph: GlobalGraph, protoNode: GraphNode<ProtoNode, ProtoDependencyMeta>) { | ||||||||||
const proto = protoNode.val.proto; | ||||||||||
if (!ClassProtoDescriptor.isClassProtoDescriptor(proto)) { | ||||||||||
return; | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Ensure consistent return types in Currently, Apply this diff: if (!ClassProtoDescriptor.isClassProtoDescriptor(proto)) {
- return;
+ return [];
} This ensures that the function always returns an array, making it easier to handle in the calling code. 📝 Committable suggestion
Suggested change
|
||||||||||
} | ||||||||||
const result: Set<GraphNode<ProtoNode, ProtoDependencyMeta>> = new Set(); | ||||||||||
const allMethods = AspectMetaBuilder.getAllMethods(proto.clazz); | ||||||||||
for (const method of allMethods) { | ||||||||||
const adviceInfoList = PointcutAdviceInfoUtil.getPointcutAdviceInfoList(proto.clazz, method); | ||||||||||
for (const { clazz } of adviceInfoList) { | ||||||||||
const property = PrototypeUtil.getProperty(clazz); | ||||||||||
assert(property, 'not found property'); | ||||||||||
const injectProto = globalGraph.findDependencyProtoNode(protoNode.val.proto, { | ||||||||||
objName: property.name, | ||||||||||
refName: property.name, | ||||||||||
qualifiers: [ | ||||||||||
...property?.qualifiers ?? [], | ||||||||||
...QualifierUtil.getProtoQualifiers(clazz), | ||||||||||
Comment on lines
+44
to
+45
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Simplify qualifiers array construction In lines 44-45, to handle the possibility that Apply this diff: qualifiers: [
- ...property?.qualifiers ?? [],
+ ...(property.qualifiers ?? []),
...QualifierUtil.getProtoQualifiers(clazz),
], This change simplifies the code and improves readability by making the handling of potential 📝 Committable suggestion
Suggested change
|
||||||||||
], | ||||||||||
}); | ||||||||||
if (injectProto) { | ||||||||||
result.add(injectProto); | ||||||||||
} | ||||||||||
} | ||||||||||
} | ||||||||||
return Array.from(result); | ||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
import { AccessLevel, SingletonProto } from "@eggjs/core-decorator"; | ||
|
||
export interface CallTraceMsg { | ||
className: string; | ||
methodName: string; | ||
id: number; | ||
name: string; | ||
result?: string; | ||
adviceParams?: any; | ||
} | ||
|
||
@SingletonProto({ | ||
accessLevel: AccessLevel.PUBLIC, | ||
}) | ||
export class CallTrace { | ||
msgs: Array<CallTraceMsg> = []; | ||
|
||
addMsg(msg: CallTraceMsg) { | ||
this.msgs.push(msg); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
import assert from 'node:assert'; | ||
import { AccessLevel, Inject } from '@eggjs/core-decorator'; | ||
import { Advice, Crosscut } from '@eggjs/aop-decorator'; | ||
import { AdviceContext, IAdvice, PointcutType } from '@eggjs/tegg-types'; | ||
import { Hello } from '../hello_succeed/Hello'; | ||
import { CallTrace } from './CallTrace'; | ||
|
||
export const crosscutAdviceParams = { | ||
cross: Math.random().toString(), | ||
cut: Math.random().toString(), | ||
}; | ||
|
||
@Crosscut({ | ||
type: PointcutType.CLASS, | ||
clazz: Hello, | ||
methodName: 'hello', | ||
}, { adviceParams: crosscutAdviceParams }) | ||
@Advice({ | ||
accessLevel: AccessLevel.PUBLIC, | ||
}) | ||
export class CrosscutAdvice implements IAdvice<Hello, string> { | ||
@Inject() | ||
callTrace: CallTrace; | ||
|
||
async beforeCall(ctx: AdviceContext<Hello, {}>): Promise<void> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid using '{}' as a type; specify an explicit interface Using Consider defining an explicit type for the context parameters: interface BeforeCallParams {
// Define the expected properties here
}
async beforeCall(ctx: AdviceContext<Hello, BeforeCallParams>): Promise<void> { 🧰 Tools🪛 Biome
|
||
assert.ok(ctx.adviceParams); | ||
assert.deepStrictEqual(ctx.adviceParams, crosscutAdviceParams); | ||
this.callTrace.addMsg({ | ||
className: CrosscutAdvice.name, | ||
methodName: 'beforeCall', | ||
id: ctx.that.id, | ||
name: ctx.args[0], | ||
adviceParams: ctx.adviceParams, | ||
}); | ||
} | ||
|
||
async afterReturn(ctx: AdviceContext<Hello>, result: any): Promise<void> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Specify the type parameter for 'AdviceContext' For consistency and better type safety, explicitly provide the second type parameter for Update the method signature: -async afterReturn(ctx: AdviceContext<Hello>, result: any): Promise<void> {
+async afterReturn(ctx: AdviceContext<Hello, AfterReturnParams>, result: any): Promise<void> { Define the
|
||
assert.ok(ctx.adviceParams); | ||
assert.deepStrictEqual(ctx.adviceParams, crosscutAdviceParams); | ||
this.callTrace.addMsg({ | ||
className: CrosscutAdvice.name, | ||
methodName: 'afterReturn', | ||
id: ctx.that.id, | ||
name: ctx.args[0], | ||
result, | ||
adviceParams: ctx.adviceParams, | ||
}); | ||
} | ||
|
||
async afterFinally(ctx: AdviceContext<Hello>): Promise<void> { | ||
assert.ok(ctx.adviceParams); | ||
assert.deepStrictEqual(ctx.adviceParams, crosscutAdviceParams); | ||
this.callTrace.addMsg({ | ||
className: CrosscutAdvice.name, | ||
methodName: 'afterFinally', | ||
id: ctx.that.id, | ||
name: ctx.args[0], | ||
adviceParams: ctx.adviceParams, | ||
}); | ||
} | ||
|
||
async around(ctx: AdviceContext<Hello>, next: () => Promise<any>): Promise<any> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Provide specific types for 'next' and return value in 'around' method The Update the method signature: -async around(ctx: AdviceContext<Hello>, next: () => Promise<any>): Promise<any> {
+async around(ctx: AdviceContext<Hello, AroundParams>, next: () => Promise<string>): Promise<string> { Ensure that
|
||
assert.ok(ctx.adviceParams); | ||
assert.deepStrictEqual(ctx.adviceParams, crosscutAdviceParams); | ||
ctx.args[0] = `withCrosscutAroundParam(${ctx.args[0]})`; | ||
const result = await next(); | ||
return `withCrossAroundResult(${result}${JSON.stringify(ctx.adviceParams)})`; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
{ | ||
"name": "hello-cross-cut", | ||
"eggModule": { | ||
"name": "helloCrossCut" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
import assert from 'node:assert'; | ||
import { AccessLevel, Inject } from '@eggjs/core-decorator'; | ||
import { Advice } from '@eggjs/aop-decorator'; | ||
import { AdviceContext, IAdvice } from '@eggjs/tegg-types'; | ||
import { Hello } from '../hello_succeed/Hello'; | ||
import { CallTrace } from '../hello_cross_cut/CallTrace'; | ||
|
||
export const pointcutAdviceParams = { | ||
point: Math.random().toString(), | ||
cut: Math.random().toString(), | ||
}; | ||
|
||
// 测试aop修改ctx的args的值 | ||
const TEST_CTX_ARGS_VALUE = 123; | ||
|
||
@Advice({ | ||
accessLevel: AccessLevel.PUBLIC, | ||
}) | ||
export class PointcutAdvice implements IAdvice<Hello> { | ||
@Inject() | ||
callTrace: CallTrace; | ||
|
||
async beforeCall(ctx: AdviceContext<Hello>): Promise<void> { | ||
assert.ok(ctx.adviceParams); | ||
assert.deepStrictEqual(ctx.adviceParams, pointcutAdviceParams); | ||
this.callTrace.addMsg({ | ||
className: PointcutAdvice.name, | ||
methodName: 'beforeCall', | ||
id: ctx.that.id, | ||
name: ctx.args[0], | ||
adviceParams: ctx.adviceParams, | ||
}); | ||
ctx.args = [...ctx.args, TEST_CTX_ARGS_VALUE]; | ||
} | ||
|
||
async afterReturn(ctx: AdviceContext<Hello>, result: any): Promise<void> { | ||
assert.ok(ctx.adviceParams); | ||
assert.deepStrictEqual(ctx.adviceParams, pointcutAdviceParams); | ||
assert.deepStrictEqual(ctx.args[ctx.args.length - 1], TEST_CTX_ARGS_VALUE); | ||
this.callTrace.addMsg({ | ||
className: PointcutAdvice.name, | ||
methodName: 'afterReturn', | ||
id: ctx.that.id, | ||
name: ctx.args[0], | ||
result, | ||
adviceParams: ctx.adviceParams, | ||
}); | ||
} | ||
|
||
async afterThrow(ctx: AdviceContext<Hello, any>, error: Error): Promise<void> { | ||
assert.ok(ctx.adviceParams); | ||
assert.deepStrictEqual(ctx.adviceParams, pointcutAdviceParams); | ||
this.callTrace.addMsg({ | ||
className: PointcutAdvice.name, | ||
methodName: 'afterThrow', | ||
id: ctx.that.id, | ||
name: ctx.args[0], | ||
result: error.message, | ||
adviceParams: ctx.adviceParams, | ||
}); | ||
} | ||
|
||
async afterFinally(ctx: AdviceContext<Hello>): Promise<void> { | ||
assert.ok(ctx.adviceParams); | ||
assert.deepStrictEqual(ctx.adviceParams, pointcutAdviceParams); | ||
this.callTrace.addMsg({ | ||
className: PointcutAdvice.name, | ||
methodName: 'afterFinally', | ||
id: ctx.that.id, | ||
name: ctx.args[0], | ||
adviceParams: ctx.adviceParams, | ||
}); | ||
} | ||
|
||
async around(ctx: AdviceContext<Hello>, next: () => Promise<any>): Promise<any> { | ||
assert.ok(ctx.adviceParams); | ||
assert.deepStrictEqual(ctx.adviceParams, pointcutAdviceParams); | ||
ctx.args[0] = `withPointAroundParam(${ctx.args[0]})`; | ||
const result = await next(); | ||
return `withPointAroundResult(${result}${JSON.stringify(pointcutAdviceParams)})`; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
{ | ||
"name": "hello-point-cut", | ||
"eggModule": { | ||
"name": "helloPointCut" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Simplify
protoNodes
Existence CheckAfter ensuring that
findCrossCuttedClazz
always returns an array, the checkif (!protoNodes)
at line 14 can be simplified sinceprotoNodes
will always be an array. You can check if the array is empty instead.Apply this diff to refine the condition:
📝 Committable suggestion