fix: Prototype should not be inherited #243
Conversation
WalkthroughThe changes introduce a new static method Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
core/core-decorator/test/fixtures/decators/ChildService.ts (1)
3-4: LGTM: Parent class is correctly defined as a singleton.The
Parentclass is properly decorated with@SingletonProto(), ensuring it follows the singleton pattern. The class is correctly exported for use in tests.Consider adding a comment to explain the purpose of this empty class in the test context. For example:
@SingletonProto() export class Parent { // This empty class is used to test singleton behavior in inheritance }core/core-decorator/test/decorators.test.ts (1)
146-149: LGTM! Consider adding more specific assertionsThe test case correctly verifies that the Child prototype does not inherit properties from the Parent prototype, which aligns with the PR objective. Good job on keeping the test focused and concise.
To make the test even more robust and informative, consider:
- Adding a comment explaining the expected behavior.
- Including more specific assertions, such as checking for particular properties.
Here's a suggested enhancement:
it('Prototype should not be inherited', () => { // Parent should have prototype properties, while Child should not inherit them const parentProps = PrototypeUtil.getProperty(Parent); const childProps = PrototypeUtil.getProperty(Child); assert(parentProps, 'Parent should have prototype properties'); assert.strictEqual(childProps, undefined, 'Child should not inherit prototype properties'); // Optional: Check for specific properties if known // assert(parentProps.name === 'expectedName', 'Parent should have the expected name'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- core/core-decorator/src/util/PrototypeUtil.ts (1 hunks)
- core/core-decorator/test/decorators.test.ts (2 hunks)
- core/core-decorator/test/fixtures/decators/ChildService.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
core/core-decorator/test/fixtures/decators/ChildService.ts (2)
1-1: LGTM: Import statement is correct.The import statement for
SingletonProtois properly defined using a relative path. This is acceptable for a test fixture.
6-6: Verify the intended behavior of inheriting from a singleton class.The
Childclass correctly extends theParentclass, which is marked as a singleton. This setup is likely intended to test singleton behavior in inheritance.However, inheriting from a singleton class might lead to unexpected behavior. Please verify if this is the intended design for your test case. If it is, consider adding a comment explaining the purpose of this inheritance structure.
To help verify the behavior, you can run the following script:
This will help us understand how the
Childclass is being used in the tests and confirm if the inheritance from a singleton is intentional.✅ Verification successful
Inheritance from Singleton Class Verified as Intended
The
Childclass extends theParentsingleton class as intended. Test cases effectively handle and verify this inheritance without introducing unexpected behaviors.
core/core-decorator/test/fixtures/decators/ChildService.tscore/core-decorator/test/util/MetadataUtil.test.tscore/core-decorator/test/decorators.test.ts🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usage of Child class to understand test context # Test: Search for Child class usage rg --type typescript "Child" core/core-decorator/testLength of output: 88
Script:
#!/bin/bash # Description: Search for usage of Child class within .ts files in the specified directory # Search for "Child" in TypeScript files rg "Child" core/core-decorator/test --include "*.ts" -A 2 -B 2Length of output: 504
Script:
#!/bin/bash # Description: Search for usage of Child class within .ts files in the specified directory using find and grep # Find all .ts files and search for "Child" with context find core/core-decorator/test -type f -name "*.ts" -exec grep -H -C 2 "Child" {} +Length of output: 2644
core/core-decorator/test/decorators.test.ts (1)
Line range hint
1-149: Overall assessment: Changes look good with minor improvements suggestedThe changes in this file are minimal and focused, aligning well with the PR objective of ensuring that prototypes are not inherited. The new test case effectively verifies this behavior. I've suggested a minor fix for a typo in the import statement and provided some recommendations to enhance the test case further. Once these small adjustments are made, the changes will be in excellent shape.
gxkl
force-pushed
the
fix/prototype-inherit
branch
from
e49f816 to
b6ac8e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
core/core-decorator/src/util/MetadataUtil.ts (1)
31-31: Consider using the class name instead ofthisin static methods.While using
thisin a static context works, it's generally considered a best practice to use the class name instead. This improves code clarity and avoids potential confusion.Consider updating the line as follows:
- return !!this.getOwnMetaData(metadataKey, clazz); + return !!MetadataUtil.getOwnMetaData(metadataKey, clazz);🧰 Tools
🪛 Biome
[error] 31-31: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
core/core-decorator/test/decorators.test.ts (2)
154-199: Approve new test suite with a minor suggestionThe new "inherited" test suite is well-structured and effectively verifies that child prototypes do not inherit properties from parent prototypes. It covers SingletonProto, StaticMultiInstanceProto, and DynamicMultiInstanceProto, which is comprehensive.
To improve maintainability, consider extracting the
fakeCtxobject to a higher scope, possibly as abeforeEachsetup, to avoid repetition and make it easier to modify if needed in the future.Here's a suggested refactor:
describe('inherited', () => { let fakeCtx; beforeEach(() => { fakeCtx = { unitPath: 'foo', moduleName: '', }; }); // ... rest of the test cases });
160-198: Approve test cases with a minor suggestion for consistencyThe test cases in the "inherited" suite are thorough and effectively verify that child prototypes do not inherit properties from parent prototypes. The use of strict equality checks is appropriate, and the test descriptions accurately reflect the purpose of each test.
For consistency, consider adding an assertion for
isEggMultiInstancePrototypein the dynamic multiInstanceProto test case, similar to the other two test cases.Add the following assertion at the beginning of the dynamic multiInstanceProto test case:
assert(PrototypeUtil.isEggMultiInstancePrototype(ParentDynamicMultiInstanceProto));core/core-decorator/test/fixtures/decators/ChildService.ts (3)
6-6: Consider Adding Decorator to 'ChildSingletonProto'The class
ChildSingletonProtoextendsParentSingletonProto, which is annotated with@SingletonProto(). IfChildSingletonProtois intended to be a singleton prototype as well, consider adding the@SingletonProto()decorator to ensure consistent behavior and clarity.Apply this diff to add the decorator:
+@SingletonProto() export class ChildSingletonProto extends ParentSingletonProto {}
22-22: Consider Adding Decorator to 'ChildDynamicMultiInstanceProto'Similarly,
ChildDynamicMultiInstanceProtoextendsParentDynamicMultiInstanceProto, which is annotated with@MultiInstanceProto(). To maintain consistency and ensure correct instantiation behavior, consider adding the@MultiInstanceProto()decorator toChildDynamicMultiInstanceProto.Apply this diff to add the decorator:
+@MultiInstanceProto() export class ChildDynamicMultiInstanceProto extends ParentDynamicMultiInstanceProto {}
1-1: Typographical Error in Directory NameIt appears that the directory name
decatorsmight be a typo. Consider renaming it todecoratorsfor clarity and to prevent any confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- core/core-decorator/src/util/MetadataUtil.ts (1 hunks)
- core/core-decorator/src/util/PrototypeUtil.ts (8 hunks)
- core/core-decorator/test/decorators.test.ts (3 hunks)
- core/core-decorator/test/fixtures/decators/ChildService.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/core-decorator/src/util/PrototypeUtil.ts
🧰 Additional context used
🪛 Biome
core/core-decorator/src/util/MetadataUtil.ts
[error] 31-31: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
🔇 Additional comments (2)
core/core-decorator/src/util/MetadataUtil.ts (2)
30-32: LGTM! New method enhances the API.The new
getOwnBooleanMetaDatamethod is a valuable addition to theMetadataUtilclass. It provides a specific method for retrieving boolean metadata, which enhances the API and improves code readability. The implementation is consistent with the class's existing methods and follows the principle of not inheriting metadata from parent classes.🧰 Tools
🪛 Biome
[error] 31-31: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Line range hint
1-33: Overall, the changes improve theMetadataUtilclass.The addition of the
getOwnBooleanMetaDatamethod enhances the API by providing a specific method for boolean metadata. This improvement aligns well with the existing class structure and functionality. The only suggestion is to use the class name instead ofthisin the static method for better clarity and adherence to best practices.🧰 Tools
🪛 Biome
[error] 27-27: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 31-31: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 35-35: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
|
Successfully published:
|
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit
New Features
Bug Fixes
Tests