fix: resolve properties's design type #208
Conversation
🦋 Changeset detectedLatest commit: ceec23d The changes in this PR will be included in the next version bump. This PR includes changesets to release 15 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
unioc
@unioc/adapter-midway
@unioc/adapter-nestjs
@unioc/commander
create-unioc
@unioc/core
@unioc/decorator
@unioc/meta
@unioc/reflector
@unioc/shared
@unioc/web
@unioc/web-bun
@unioc/web-express
@unioc/web-koa
commit: |
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in property design type resolution by adding proper validation and fallback logic when resolving dependencies for properties decorated with @Inject.
Changes:
- Adds validation to check if design type exists in container before attempting to create a new instance
- Adds
isClassvalidation to ensure design type is actually a class before attempting instantiation - Implements proper error handling for non-optional properties with invalid design types
- Handles optional properties gracefully by setting them to undefined when design type is invalid
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/core/core/src/contexts/classes/class-factory.ts | Refactors _resolveInjected method to add multi-step validation and resolution logic for property design types, including container lookup, class validation, and optional property handling |
| .changeset/floppy-towns-invent.md | Adds changeset entry documenting the fix (contains a minor spelling error) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const currentDesignType = this.getClassWrapper() | ||
| .getMetadataScanner() | ||
| .getPropertyDesignType(item.propertyKey) | ||
| .getPropertyDesignType(item.propertyKey) as unknown |
There was a problem hiding this comment.
The type assertion as unknown appears unnecessary here. The getPropertyDesignType method already returns IClass | undefined according to the interface definition. This type assertion weakens type safety without providing clear benefits. Consider removing it or explaining why it's needed.
| } | ||
| } | ||
| else { | ||
| throw new Error(`Property ${item.propertyKey.toString()} is decorated with @Inject, but the property is not a class property.`) |
There was a problem hiding this comment.
The error message format is inconsistent with other errors in this method. Lines 84 and 100 use the format Property "{ClassName}.{propertyKey}", but this line uses Property {propertyKey} without the class name prefix. For consistency and better debugging, this should match the pattern used in the other error messages.
| throw new Error(`Property ${item.propertyKey.toString()} is decorated with @Inject, but the property is not a class property.`) | |
| throw new Error(`Property "${this.getClassWrapper().getTarget().name}.${item.propertyKey.toString()}" is decorated with @Inject, but the property is not a class property.`) |
| "@unioc/core": patch | ||
| --- | ||
|
|
||
| fix: resolve properties's design type |
There was a problem hiding this comment.
Incorrect possessive form. "properties's" should be "property's" (if referring to a single property) or "properties'" (if referring to multiple properties). Based on the context of the PR fixing property design type resolution, "property's" is likely the correct form.
| fix: resolve properties's design type | |
| fix: resolve property's design type |
| continue | ||
| } | ||
| else if (isClass(currentDesignType)) { | ||
| const classWrapper = this.getClassWrapper().getBootstrap().createClass(currentDesignType) | ||
| instance[item.propertyKey] = await classWrapper.resolve() | ||
| continue | ||
| } | ||
| else if (item.optional === false) { | ||
| throw new Error(`Property "${this.getClassWrapper().getTarget().name}.${item.propertyKey.toString()}" is decorated with @Inject, but the design type "${currentDesignType.toString()}" is not found.`) | ||
| } | ||
| else { | ||
| instance[item.propertyKey] = undefined | ||
| continue |
There was a problem hiding this comment.
The continue statements on lines 92, 97, and 104 are unnecessary because they are at the end of their respective conditional blocks within the loop. Removing them would make the code cleaner without changing behavior, as the loop naturally continues to the next iteration after completing each block.
| continue | |
| } | |
| else if (isClass(currentDesignType)) { | |
| const classWrapper = this.getClassWrapper().getBootstrap().createClass(currentDesignType) | |
| instance[item.propertyKey] = await classWrapper.resolve() | |
| continue | |
| } | |
| else if (item.optional === false) { | |
| throw new Error(`Property "${this.getClassWrapper().getTarget().name}.${item.propertyKey.toString()}" is decorated with @Inject, but the design type "${currentDesignType.toString()}" is not found.`) | |
| } | |
| else { | |
| instance[item.propertyKey] = undefined | |
| continue | |
| } | |
| else if (isClass(currentDesignType)) { | |
| const classWrapper = this.getClassWrapper().getBootstrap().createClass(currentDesignType) | |
| instance[item.propertyKey] = await classWrapper.resolve() | |
| } | |
| else if (item.optional === false) { | |
| throw new Error(`Property "${this.getClassWrapper().getTarget().name}.${item.propertyKey.toString()}" is decorated with @Inject, but the design type "${currentDesignType.toString()}" is not found.`) | |
| } | |
| else { | |
| instance[item.propertyKey] = undefined |
| const foundWrapper = GLOBAL_CONTAINER.findOne(currentDesignType as IContainer.InjectionToken) | ||
| if (foundWrapper) { | ||
| instance[item.propertyKey] = await foundWrapper.resolve() | ||
| continue | ||
| } | ||
| else if (isClass(currentDesignType)) { | ||
| const classWrapper = this.getClassWrapper().getBootstrap().createClass(currentDesignType) | ||
| instance[item.propertyKey] = await classWrapper.resolve() | ||
| continue | ||
| } | ||
| else if (item.optional === false) { | ||
| throw new Error(`Property "${this.getClassWrapper().getTarget().name}.${item.propertyKey.toString()}" is decorated with @Inject, but the design type "${currentDesignType.toString()}" is not found.`) | ||
| } | ||
| else { | ||
| instance[item.propertyKey] = undefined | ||
| continue | ||
| } |
There was a problem hiding this comment.
The new logic branches introduced in this PR lack test coverage. The existing test only covers the case where an explicit injection token is provided (line 24 of index.test.ts). Consider adding tests for: 1) design type found in container, 2) design type that is a class but not registered in container, 3) optional properties with non-class design types, and 4) non-optional properties with invalid design types to verify error handling.
| continue | ||
| } | ||
| else if (item.optional === false) { | ||
| throw new Error(`Property "${this.getClassWrapper().getTarget().name}.${item.propertyKey.toString()}" is decorated with @Inject, but the design type "${currentDesignType.toString()}" is not found.`) |
There was a problem hiding this comment.
Calling .toString() on currentDesignType in the error message may not produce meaningful output for all cases. When TypeScript's reflection metadata returns primitive constructors (String, Number, Boolean) or Object for complex types, .toString() will show function String() { [native code] } or similar. Consider using typeof currentDesignType === 'function' ? currentDesignType.name : String(currentDesignType) to make the error message more readable.
| throw new Error(`Property "${this.getClassWrapper().getTarget().name}.${item.propertyKey.toString()}" is decorated with @Inject, but the design type "${currentDesignType.toString()}" is not found.`) | |
| throw new Error(`Property "${this.getClassWrapper().getTarget().name}.${item.propertyKey.toString()}" is decorated with @Inject, but the design type "${typeof currentDesignType === 'function' ? currentDesignType.name : String(currentDesignType)}" is not found.`) |
No description provided.