-
Notifications
You must be signed in to change notification settings - Fork 38
fix: fix miss MultiInstance proper qualifiers #241
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
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 |
|---|---|---|
|
|
@@ -34,6 +34,7 @@ export class EggPrototypeBuilder { | |
| private injectObjects: Array<InjectObject | InjectConstructor> = []; | ||
| private loadUnit: LoadUnit; | ||
| private qualifiers: QualifierInfo[] = []; | ||
| private properQualifiers: Record<string, QualifierInfo[]> = {}; | ||
|
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. Inconsistent naming: 'properQualifiers' vs 'propertyQualifiers' There is inconsistency in variable names between |
||
| private className?: string; | ||
| private multiInstanceConstructorIndex?: number; | ||
| private multiInstanceConstructorAttributes?: QualifierAttribute[]; | ||
|
|
@@ -56,38 +57,47 @@ 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); | ||
| return builder.build(); | ||
| } | ||
|
|
||
| private tryFindDefaultPrototype(injectObject: InjectObject): EggPrototype { | ||
| const propertyQualifiers = QualifierUtil.getProperQualifiers(this.clazz, injectObject.refName); | ||
| return EggPrototypeFactory.instance.getPrototype(injectObject.objName, this.loadUnit, propertyQualifiers); | ||
| 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, | ||
| ]); | ||
|
Comment on lines
+69
to
+74
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 code duplication in prototype retrieval The logic for retrieving qualifiers and calling Suggested refactoring: Add a private helper method to combine qualifiers: private getCombinedQualifiers(injectObject: InjectObject): QualifierInfo[] {
const propertyQualifiers = QualifierUtil.getProperQualifiers(this.clazz, injectObject.refName);
const multiInstancePropertyQualifiers = this.properQualifiers[injectObject.refName as string] ?? [];
return [
...propertyQualifiers,
...multiInstancePropertyQualifiers,
];
}Modify the methods to use this helper: // In tryFindDefaultPrototype
-const propertyQualifiers = QualifierUtil.getProperQualifiers(this.clazz, injectObject.refName);
-const multiInstancePropertyQualifiers = this.properQualifiers[injectObject.refName as string] ?? [];
return EggPrototypeFactory.instance.getPrototype(injectObject.objName, this.loadUnit, [
- ...propertyQualifiers,
- ...multiInstancePropertyQualifiers,
+ ...this.getCombinedQualifiers(injectObject),
]);
// Similar changes in tryFindContextPrototype and tryFindSelfInitTypePrototypeAlso applies to: 78-80, 91-93 |
||
| } | ||
|
|
||
| private tryFindContextPrototype(injectObject: InjectObject): EggPrototype { | ||
| let propertyQualifiers = QualifierUtil.getProperQualifiers(this.clazz, injectObject.refName); | ||
| propertyQualifiers = [ | ||
| const propertyQualifiers = QualifierUtil.getProperQualifiers(this.clazz, injectObject.refName); | ||
| const multiInstancePropertyQualifiers = this.properQualifiers[injectObject.refName as string] ?? []; | ||
| return EggPrototypeFactory.instance.getPrototype(injectObject.objName, this.loadUnit, [ | ||
| ...propertyQualifiers, | ||
| ...multiInstancePropertyQualifiers, | ||
| { | ||
| attribute: InitTypeQualifierAttribute, | ||
| value: ObjectInitType.CONTEXT, | ||
| }, | ||
| ]; | ||
| return EggPrototypeFactory.instance.getPrototype(injectObject.objName, this.loadUnit, propertyQualifiers); | ||
| ]); | ||
| } | ||
|
|
||
| private tryFindSelfInitTypePrototype(injectObject: InjectObject): EggPrototype { | ||
| let propertyQualifiers = QualifierUtil.getProperQualifiers(this.clazz, injectObject.refName); | ||
| propertyQualifiers = [ | ||
| const propertyQualifiers = QualifierUtil.getProperQualifiers(this.clazz, injectObject.refName); | ||
| const multiInstancePropertyQualifiers = this.properQualifiers[injectObject.refName as string] ?? []; | ||
| return EggPrototypeFactory.instance.getPrototype(injectObject.objName, this.loadUnit, [ | ||
| ...propertyQualifiers, | ||
| ...multiInstancePropertyQualifiers, | ||
| { | ||
| attribute: InitTypeQualifierAttribute, | ||
| value: this.initType, | ||
| }, | ||
| ]; | ||
| return EggPrototypeFactory.instance.getPrototype(injectObject.objName, this.loadUnit, propertyQualifiers); | ||
| ]); | ||
| } | ||
|
|
||
| private findInjectObjectPrototype(injectObject: InjectObject): EggPrototype { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -55,17 +55,18 @@ export class ClazzMap { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert(property, `multi instance property not found for ${clazz.name}`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (const info of property.objects) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const instanceQualifiers = [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ...qualifiers, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ...info.qualifiers, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+58
to
+61
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 When using the spread operator on Consider providing a default empty array if const instanceQualifiers = [
...qualifiers,
- ...info.qualifiers,
+ ...(info.qualifiers || []),
];📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clazzMap[info.name] = clazzMap[info.name] || []; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clazzMap[info.name].push({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| name: info.name, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| accessLevel: PrototypeUtil.getAccessLevel(clazz, { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| unitPath: instanceNode.val.moduleConfig.path, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| moduleName: instanceNode.val.moduleConfig.name, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) as AccessLevel, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| qualifiers: [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ...qualifiers, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ...info.qualifiers, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| qualifiers: instanceQualifiers, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| properQualifiers: info.properQualifiers || {}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| instanceModule: instanceNode, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ownerModule: ownerNode, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -179,18 +180,20 @@ export class ModuleNode implements GraphNodeObj { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!this.clazzList.includes(clazz)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.clazzList.push(clazz); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const defaultQualifier = [{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| attribute: InitTypeQualifierAttribute, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| value: PrototypeUtil.getInitType(clazz, { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| unitPath: this.moduleConfig.path, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| moduleName: this.moduleConfig.name, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| })!, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| attribute: LoadUnitNameQualifierAttribute, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| value: this.name, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (const qualifier of defaultQualifier) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| QualifierUtil.addProtoQualifier(clazz, qualifier.attribute, qualifier.value); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!PrototypeUtil.isEggMultiInstancePrototype(clazz)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const defaultQualifier = [{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| attribute: InitTypeQualifierAttribute, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| value: PrototypeUtil.getInitType(clazz, { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| unitPath: this.moduleConfig.path, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| moduleName: this.moduleConfig.name, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| })!, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| attribute: LoadUnitNameQualifierAttribute, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| value: this.name, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (const qualifier of defaultQualifier) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| QualifierUtil.addProtoQualifier(clazz, qualifier.attribute, qualifier.value); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+183
to
+196
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. Handle possible The Consider checking if const initType = PrototypeUtil.getInitType(clazz, {
unitPath: this.moduleConfig.path,
moduleName: this.moduleConfig.name,
});
+if (!initType) {
+ // Handle undefined initType appropriately
+ throw new Error(`InitType is undefined for ${clazz.name}`);
+}
const defaultQualifier = [{
attribute: InitTypeQualifierAttribute,
- value: PrototypeUtil.getInitType(clazz, {
- unitPath: this.moduleConfig.path,
- moduleName: this.moduleConfig.name,
- })!,
+ value: initType,
}, {
attribute: LoadUnitNameQualifierAttribute,
value: this.name,
}];📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,9 @@ | ||
| BizManager: | ||
| clients: | ||
| foo: {} | ||
| bar: {} | ||
| foo: | ||
| secret: '1' | ||
| bar: | ||
| secret: '2' | ||
|
|
||
| secret: | ||
| keys: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,10 @@ | ||
| import { Inject, SingletonProto } from '@eggjs/core-decorator'; | ||
| import { Inject, ModuleQualifier, SingletonProto } from '@eggjs/core-decorator'; | ||
| import { Secret, SecretQualifier } from '../foo/Secret'; | ||
|
|
||
| @SingletonProto() | ||
| export class App2 { | ||
| @Inject() | ||
| @SecretQualifier('app2') | ||
| @ModuleQualifier('app2') | ||
| @SecretQualifier('1') | ||
| secret: Secret; | ||
| } |
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.
Ensure
object.qualifiersis initialized before usageThe code assumes that
object.qualifiersis already an array. Ifobject.qualifiersisundefined, accessingfindor pushing to it will result in a runtime error. It's important to ensure thatobject.qualifiersis initialized.Apply this diff to initialize
object.qualifiersif it's not already:for (const object of objects) { + if (!object.qualifiers) { + object.qualifiers = []; + } defaultQualifier.forEach(qualifier => { if (!object.qualifiers.find(t => t.attribute === qualifier.attribute)) { object.qualifiers.push(qualifier); } }); }📝 Committable suggestion