Skip to content
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: add default inject init type qualifier #255

Merged
merged 2 commits into from
Oct 30, 2024

Conversation

gxkl
Copy link
Contributor

@gxkl gxkl commented Oct 28, 2024

Checklist
  • npm test passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change

Summary by CodeRabbit

  • New Features

    • Enhanced dependency injection capabilities with new services and qualifiers.
    • Introduced multiple new classes and methods to improve service management.
    • Added configuration files for better plugin management.
  • Bug Fixes

    • Updated test cases to reflect changes in expected outputs for dependency injection.
  • Documentation

    • Added metadata in new package.json files for modules and services, improving clarity on module structure.
  • Tests

    • Expanded test coverage for new services and qualifiers to ensure correct functionality.

Copy link

coderabbitai bot commented Oct 28, 2024

Walkthrough

The changes in this pull request primarily involve enhancements to the dependency injection framework within the Egg.js architecture. A new function, guessInjectInfo, has been introduced to streamline the logic for determining injection parameters in the Inject decorator. Additionally, new methods in the PrototypeUtil class and various test enhancements were made to improve coverage and validate new functionalities. Several new classes and properties related to dependency injection were added, along with configuration files for managing plugins and modules.

Changes

File Change Summary
core/core-decorator/src/decorator/Inject.ts Added function guessInjectInfo; modified Inject and InjectOptional functions to utilize the new logic.
core/core-decorator/src/util/PrototypeUtil.ts Added static method getDesignParamtypes to retrieve parameter type metadata.
core/core-decorator/test/decorators.test.ts Expanded test suite with new cases for qualifiers and updated existing tests for constructor injection.
core/core-decorator/test/fixtures/decators/ConstructorObject.ts Added classes CacheService, CacheContextService, and ConstructorQualifierObject; updated ConstructorObject constructor.
core/core-decorator/test/fixtures/decators/QualifierCacheService.ts Introduced classes TestContextService, TestSingletonService; added properties to CacheService.
core/metadata/src/factory/EggPrototypeCreatorFactory.ts Modified createProto method to streamline qualifier handling.
plugin/tegg/test/Inject.test.ts Added new test suite for BarService and BarConstructorService; verified functionality of new services.
plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/*.ts Added classes BarService1, BarService2, BarConstructorService1, BarConstructorService2, and FooService, all with dependency injection.
plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/config/*.js Introduced configuration files for plugin management.
plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/package.json Added new package.json files for module and project metadata.

Possibly related PRs

Suggested reviewers

  • killagu

🐰 "In the garden of code we hop,
With decorators that never stop.
New services bloom, dependencies grow,
Injections refined, watch our framework glow!
From tests to configs, all in a row,
Let's celebrate changes, let the good times flow!" 🐇


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@gxkl gxkl requested a review from killagu October 28, 2024 14:14
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 8

🧹 Outside diff range and nitpick comments (15)
plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/FooService.ts (1)

3-6: Consider adding JSDoc documentation.

Since this is a test fixture class demonstrating the new default inject init type qualifier feature, it would be helpful to add JSDoc comments explaining its purpose and how it fits into the test scenarios.

+/**
+ * Test fixture class demonstrating context-level dependency injection.
+ * Used in conjunction with BarConstructorService1 and BarConstructorService2
+ * to verify proper injection behavior with PUBLIC access level.
+ */
 @ContextProto({ accessLevel: AccessLevel.PUBLIC })
 export class FooService {
   type = 'context';
 }
plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-foo/FooService.ts (1)

3-6: Consider enhancing the test fixture with additional scenarios.

Since this is a test fixture for singleton vs context prototype behavior, consider adding methods or properties that would help verify singleton behavior, such as a counter or timestamp to ensure the same instance is being reused.

Example enhancement:

@SingletonProto({ accessLevel: AccessLevel.PUBLIC })
export class FooService {
  type = 'singleton';
+  private readonly instanceId = Date.now();
+  getInstanceId() {
+    return this.instanceId;
+  }
}
plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarService2.ts (1)

9-11: Add null check for fooService.

The type() method directly accesses fooService.type without checking if fooService is properly initialized. Consider adding a null check to handle potential undefined states during testing.

 type() {
+  if (!this.fooService) {
+    throw new Error('FooService not initialized');
+  }
   return this.fooService.type;
 }
plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarService1.ts (2)

6-7: Consider adding JSDoc for the injected dependency.

While the code is correct, adding documentation for the injected dependency would improve maintainability.

+  /**
+   * Injected FooService instance
+   */
   @Inject()
   fooService: FooService;

9-11: Add return type annotation for better type safety.

The method implementation is correct, but adding a return type would improve type safety and documentation.

-  type() {
+  type(): string {
     return this.fooService.type;
   }
plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarConstructorService2.ts (1)

10-12: Consider adding JSDoc comments.

Since this is a test fixture that demonstrates important DI behavior, adding JSDoc comments would help other developers understand the purpose of this method.

+  /**
+   * Returns the type from the injected FooService
+   * Used to verify correct dependency injection behavior
+   */
   type() {
     return this.fooService.type;
   }
plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarConstructorService1.ts (1)

10-12: Consider adding return type annotation.

For better type safety and documentation, consider adding a return type annotation to the type() method.

-  type() {
+  type(): string {
     return this.fooService.type;
   }
plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/config/config.default.js (2)

1-1: Remove redundant 'use strict' directive.

Since this is a module, the 'use strict' directive is redundant as modules are automatically in strict mode.

-'use strict';
🧰 Tools
🪛 Biome

[error] 1-1: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)


5-20: LGTM with a minor suggestion for test keys.

The configuration structure is well-organized and follows Egg.js conventions. The CSRF protection and logger configurations are properly set up.

Consider using a more distinctive test key to prevent accidental reuse:

-    keys: 'test key',
+    keys: 'same-name-singleton-and-context-proto-test-key',
core/core-decorator/test/fixtures/decators/QualifierCacheService.ts (1)

Line range hint 1-38: Consider adding JSDoc comments

While the test cases are well-structured, adding JSDoc comments to explain the purpose of each test case would make the test fixture more maintainable and serve as documentation for other contributors.

Example addition:

+/**
+ * Test fixture demonstrating various injection patterns with InitTypeQualifier.
+ * Covers:
+ * - Basic interface injection
+ * - Concrete class injection
+ * - Named injection
+ * - InitTypeQualifier ordering variations
+ */
@ContextProto()
export default class CacheService {
plugin/tegg/test/Inject.test.ts (2)

6-13: Consider standardizing import statement style.

The import statements mix single-line and multi-line styles. For consistency, consider using the same style throughout.

-import { BarService1 } from './fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarService1';
-import { BarService2 } from './fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarService2';
-import {
-  BarConstructorService1,
-} from './fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarConstructorService1';
-import {
-  BarConstructorService2,
-} from './fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarConstructorService2';
+import { BarService1 } from './fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarService1';
+import { BarService2 } from './fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarService2';
+import { BarConstructorService1 } from './fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarConstructorService1';
+import { BarConstructorService2 } from './fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarConstructorService2';

58-94: Consider adding edge case tests.

The current test suite covers the happy path scenarios well. Consider adding tests for:

  • Error cases when services are accessed outside of module context
  • Behavior when services are requested multiple times (verify singleton behavior)
  • Concurrent access patterns for context-scoped services

Would you like me to help generate these additional test cases?

core/core-decorator/src/util/PrototypeUtil.ts (1)

288-290: Consider improving type safety of the return value.

The implementation looks good and follows the established pattern. However, the return type could be more specific than unknown[] to better support type inference in the dependency injection system.

Consider updating the return type to be more specific:

-  static getDesignParamtypes(clazz: EggProtoImplClass, propKey?: PropertyKey) {
-    return MetadataUtil.getMetaData<unknown[]>('design:paramtypes', clazz, propKey);
+  static getDesignParamtypes(clazz: EggProtoImplClass, propKey?: PropertyKey): (Function | undefined)[] | undefined {
+    return MetadataUtil.getMetaData<(Function | undefined)[]>('design:paramtypes', clazz, propKey);
+  }

This change:

  1. Makes it clear that the method can return undefined if no metadata exists
  2. Specifies that the array contains constructor functions (which is what TypeScript emits for parameter types)
  3. Allows undefined elements for parameters that can't be resolved to a type
core/core-decorator/test/fixtures/decators/ConstructorObject.ts (1)

35-36: Ensure consistent decorator ordering for readability

The order of decorators differs between customQualifierCache1 and customQualifierCache2. While it may not affect functionality, maintaining a consistent order improves code readability.

Consider updating the decorator order for customQualifierCache2:

-    @Inject() @InitTypeQualifier(ObjectInitType.CONTEXT) readonly customQualifierCache2: CacheService,
+    @InitTypeQualifier(ObjectInitType.CONTEXT) @Inject() readonly customQualifierCache2: CacheService,
core/core-decorator/src/decorator/Inject.ts (1)

Line range hint 41-65: Consistent Handling of initType in propertyInject

When initType is determined in propertyInject, ensure that it is consistently applied, especially when injectParam is provided. If injectParam includes an initType, consider how it should interact with the inferred initType from guessInjectInfo.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 260470b and e406340.

📒 Files selected for processing (18)
  • core/core-decorator/src/decorator/Inject.ts (3 hunks)
  • core/core-decorator/src/util/PrototypeUtil.ts (1 hunks)
  • core/core-decorator/test/decorators.test.ts (5 hunks)
  • core/core-decorator/test/fixtures/decators/ConstructorObject.ts (2 hunks)
  • core/core-decorator/test/fixtures/decators/QualifierCacheService.ts (2 hunks)
  • core/metadata/src/factory/EggPrototypeCreatorFactory.ts (2 hunks)
  • plugin/tegg/test/Inject.test.ts (2 hunks)
  • plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarConstructorService1.ts (1 hunks)
  • plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarConstructorService2.ts (1 hunks)
  • plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarService1.ts (1 hunks)
  • plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarService2.ts (1 hunks)
  • plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/FooService.ts (1 hunks)
  • plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/package.json (1 hunks)
  • plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-foo/FooService.ts (1 hunks)
  • plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-foo/package.json (1 hunks)
  • plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/config/config.default.js (1 hunks)
  • plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/config/plugin.js (1 hunks)
  • plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/package.json (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-foo/package.json
  • plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/package.json
🧰 Additional context used
🪛 Biome
plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/config/config.default.js

[error] 1-1: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)

plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/config/plugin.js

[error] 1-1: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)

🔇 Additional comments (30)
plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/package.json (1)

1-6: Verify the intentional module name mismatch.

The module is named "module-a" but resides in a "module-bar" directory. While this mismatch could be intentional for testing name resolution or conflicts, it would be helpful to document the purpose of this test fixture to avoid confusion.

Consider adding a comment in the package.json to explain:

 {
   "name": "module-a",
+  "description": "Test fixture for verifying module name resolution between singleton and context prototypes",
   "eggModule": {
     "name": "a"
   }
 }

Let's verify if there are other similar test fixtures:

plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/FooService.ts (1)

1-6: LGTM! Clean and focused test fixture implementation.

The implementation is well-structured and appropriately uses the @ContextProto decorator with PUBLIC access level, serving as a good test case for dependency injection scenarios.

plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-foo/FooService.ts (2)

1-6: LGTM! Clean and focused test fixture implementation.

The implementation follows best practices for dependency injection in Egg.js:

  • Proper use of @SingletonProto decorator with explicit access level
  • Clear and descriptive naming
  • Focused test fixture that serves a single purpose

1-1: Verify the dependency on @eggjs/tegg package.

The import statement suggests a dependency on @eggjs/tegg. Let's verify this dependency is properly declared in the package.json.

✅ Verification successful

The dependency on @eggjs/tegg is properly declared and used in test fixtures

This is a test fixture within the @eggjs/tegg-plugin package itself, which has @eggjs/tegg properly declared as a dependency in its package.json. The import is valid since this is part of the test suite for the tegg plugin. The minimal package.json in the test fixture is intentional as it's just a test app.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if @eggjs/tegg is declared as a dependency
jq -r '.dependencies["@eggjs/tegg"] // .devDependencies["@eggjs/tegg"] // empty' package.json

# Verify the import path exists in node_modules
fd -t f "SingletonProto.{ts,js,d.ts}" node_modules/@eggjs/tegg

Length of output: 272


Script:

#!/bin/bash
# Let's check the package.json in the root and any potential package.json files in subdirectories
fd -t f "package.json" . -x cat {}

# Also check if this is a test fixture by looking at its path structure
pwd

Length of output: 66892


Script:

#!/bin/bash
# Since this is a test fixture, let's check the package.json in the test directory and its parent directories
pwd
ls -la plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/package.json || echo "No package.json in app dir"
ls -la plugin/tegg/package.json || echo "No package.json in plugin/tegg"

Length of output: 352


Script:

#!/bin/bash
# Let's check the content of both package.json files
cat plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/package.json
echo "---"
cat plugin/tegg/package.json

Length of output: 1990

plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/config/plugin.js (3)

3-6: LGTM! Tracer plugin configuration is correct.

The configuration follows the standard Egg.js plugin format and properly enables the egg-tracer plugin.


8-11: LGTM! TEgg config plugin configuration is correct.

The configuration properly enables the @eggjs/tegg-config plugin using the standard format.


13-13: LGTM! Watcher configuration is appropriate for test fixture.

Explicitly disabling the watcher is a reasonable choice for a test fixture.

plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarService2.ts (2)

1-12: LGTM for test fixture structure.

The overall structure of the test fixture is good:

  • Proper use of decorators
  • Clear dependency injection pattern
  • Consistent with testing same-name singleton and context prototype scenarios

6-7: Consider specifying init type qualifier in @Inject decorator.

Given that this PR aims to add default inject init type qualifiers, consider explicitly specifying the init type for clarity and to align with the new feature.

plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarService1.ts (2)

1-2: LGTM! Imports are clean and properly utilized.


4-5: LGTM! Proper singleton pattern implementation.

The class is correctly decorated as a singleton prototype and follows naming conventions.

Let's verify if there's a corresponding context prototype version of this service:

plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarConstructorService2.ts (2)

4-13: LGTM! Clean implementation of a singleton service with constructor injection.

The class follows best practices for dependency injection in Egg.js:

  • Properly decorated with @SingletonProto()
  • Uses constructor injection which is preferable for required dependencies
  • Clear and focused responsibility

6-8: Consider adding parameter type qualifier.

Given that this PR is about adding default inject init type qualifier, you might want to explicitly specify the injection type for the fooService parameter to demonstrate the new functionality.

Let's check if other test files use type qualifiers:

plugin/tegg/test/fixtures/apps/same-name-singleton-and-context-proto/app/modules/module-bar/BarConstructorService1.ts (2)

1-4: LGTM! Clean imports and proper decorator usage.

The imports are well-organized and the SingletonProto decorator is correctly applied.


5-8: Verify if default type qualifier is needed.

The constructor injection is well-implemented with readonly modifier. However, given that this PR is about adding default inject init type qualifier, should we explicitly specify a type qualifier here?

Let's check how type qualifiers are used in other files:

✅ Verification successful

Based on the search results, I can now generate the final response:

No type qualifier needed for this basic injection case

The codebase shows that @Inject() type qualifiers are only used in specific cases:

  • { optional: true } for optional dependencies
  • { name: 'serviceName' } for named service injection
  • String literals for direct service name injection

Since this is a basic required dependency injection without any special requirements, the current implementation without a type qualifier is correct and follows the codebase patterns.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other @Inject usages with type qualifiers
rg -l "@Inject\([^)]+\)" | while read -r file; do
  echo "=== $file ==="
  rg "@Inject\([^)]+\)" "$file"
done

Length of output: 3770

core/core-decorator/test/fixtures/decators/QualifierCacheService.ts (3)

2-2: LGTM: Well-structured test service classes

The test service classes are appropriately decorated with their respective scopes (@ContextProto() and @SingletonProto()), providing clear test cases for different initialization types.

Also applies to: 5-9


20-30: LGTM: Comprehensive injection test cases

Good coverage of various injection scenarios including interface injection, concrete class injection, and named injection.


32-38: Verify decorator order behavior

The test includes two variations of combining @InitTypeQualifier and @Inject decorators with different ordering:

  1. @InitTypeQualifier before @Inject
  2. @InitTypeQualifier after @Inject

This is good for testing, but we should verify that the order doesn't affect the behavior.

✅ Verification successful

Decorator order does not affect behavior

After analyzing the implementation in QualifierUtil, I can confirm that the order of decorators doesn't matter because:

  1. The InitTypeQualifier decorator stores its metadata in a Map using QualifierUtil.addInjectQualifier(), where each qualifier is uniquely identified by its attribute
  2. The metadata storage is independent of decorator execution order since it uses a key-value store where newer values for the same key (attribute) will override older ones
  3. Both variations in the test file (QualifierCacheService.ts) will result in the same metadata being stored, regardless of whether @InitTypeQualifier is placed before or after @Inject

The test variations are correctly verifying that the functionality works consistently in both ordering scenarios.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other instances of InitTypeQualifier usage to verify consistent ordering
rg "@InitTypeQualifier" -B 2 -A 2

# Search for any documentation or tests specifically about decorator ordering
rg -g "*.md" -g "*.test.ts" "decorator.*order" -i

Length of output: 4362


Script:

#!/bin/bash
# Let's search for the InitTypeQualifier decorator implementation to understand how it's processed
ast-grep --pattern 'export function InitTypeQualifier($$$) {
  $$$
}'

# Also search for any test cases that verify the behavior
rg -g "*.test.ts" "InitTypeQualifier.*test|describe.*InitTypeQualifier" -B 2 -A 10

Length of output: 1192


Script:

#!/bin/bash
# Let's look for the QualifierUtil implementation to understand how qualifiers are processed
ast-grep --pattern 'class QualifierUtil {
  $$$
  addInjectQualifier($$$) {
    $$$
  }
  $$$
}'

# Also search for any test assertions related to these decorators
rg -g "*.test.ts" "expect.*customQualifierService|test.*InitTypeQualifier" -B 2 -A 5

Length of output: 8837

plugin/tegg/test/Inject.test.ts (1)

58-94: Well-structured test suite with good coverage.

The test suite effectively validates both singleton and context prototype behavior:

  • Good use of mockModuleContextScope for context simulation
  • Proper TypeScript type annotations
  • Clear assertions checking prototype types
  • Comprehensive coverage of both property and constructor injection cases
core/metadata/src/factory/EggPrototypeCreatorFactory.ts (1)

Line range hint 26-69: Well-structured changes that improve maintainability.

The refactoring aligns well with the PR objective of adding default inject init type qualifier. The consistent handling of qualifiers across both multi-instance and single-instance prototypes reduces code duplication and improves maintainability.

A few architectural considerations:

  1. The qualifier initialization logic could potentially be moved to a separate method for better reusability
  2. Consider adding unit tests to verify the behavior with undefined/null qualifiers

Let's check the test coverage:

#!/bin/bash
# Search for relevant test cases
rg -l "EggPrototypeCreatorFactory.*qualifiers" --type ts
core/core-decorator/test/decorators.test.ts (2)

19-19: LGTM: Import follows established patterns

The addition of ConstructorQualifierObject import is well-placed and follows the existing naming conventions.


93-95: LGTM: Constructor injection assertions are well-structured

The modified assertions maintain consistency with the testing pattern while extending coverage for the new qualifier functionality.

core/core-decorator/src/util/PrototypeUtil.ts (1)

288-290: Verify the integration with guessInjectInfo.

The method appears to be part of a larger feature for improving type inference in dependency injection. Let's verify its usage in the codebase.

✅ Verification successful

Based on the shell script results, I can now generate the final response:

Integration with guessInjectInfo is properly implemented

The getDesignParamtypes method is correctly integrated with guessInjectInfo in the dependency injection flow:

  • Used in @Inject decorator to retrieve parameter types for constructor injection
  • The types are passed to guessInjectInfo which uses them to determine injection metadata
  • Proper fallback mechanisms exist when type information is unavailable
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usages of getDesignParamtypes
echo "Searching for direct usage of getDesignParamtypes:"
rg "getDesignParamtypes" -A 5

echo -e "\nSearching for guessInjectInfo implementation:"
rg "guessInjectInfo" -A 10

Length of output: 3874

core/core-decorator/test/fixtures/decators/ConstructorObject.ts (5)

1-8: Import statements are correctly updated

The added import for ContextProto and adjustments to existing imports are appropriate.


9-11: CacheService class definition is accurate

The CacheService class is properly defined with the @SingletonProto() decorator.


12-13: CacheContextService class definition is accurate

The CacheContextService class is correctly defined with the @ContextProto() decorator.


25-25: Optional injection is correctly implemented

The use of @InjectOptional() for optional2 is appropriate and follows the pattern for optional dependencies.


22-22: Verify the addition of otherCache in ConstructorObject

The injection of otherCache as a CacheService dependency in ConstructorObject looks correct. Ensure that all instances where ConstructorObject is instantiated are updated to accommodate this new dependency.

To confirm, run the following script to find instantiations of ConstructorObject:

✅ Verification successful

No updates needed for ConstructorObject instantiations

Based on the codebase analysis, ConstructorObject is only used in test files and specifically for testing the decorator functionality. It's not instantiated anywhere in the codebase, as it's a test fixture class used to verify the behavior of decorators like @Inject, @InitTypeQualifier, etc.

The test file decorators.test.ts already properly handles the new otherCache dependency in its verification, as seen in the test:

assert.deepStrictEqual(injectConstructors, [
  { refIndex: 0, refName: 'xCache', objName: 'fooCache' },
  { refIndex: 1, refName: 'cache', objName: 'cache' },
  { refIndex: 2, refName: 'otherCache', objName: 'cacheService' },
  { refIndex: 3, refName: 'optional1', objName: 'optional1', optional: true },
  { refIndex: 4, refName: 'optional2', objName: 'optional2', optional: true },
]);
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all instantiations of ConstructorObject to verify constructor usage.

# Search for 'new ConstructorObject(' across the codebase.
rg 'new\s+ConstructorObject\('

Length of output: 32


Script:

#!/bin/bash
# Let's try a more comprehensive search strategy

# 1. First, let's find all files that mention ConstructorObject
rg "ConstructorObject" -l

# 2. Let's also see the actual test file content to understand how it's being used
cat core/core-decorator/test/fixtures/decators/ConstructorObject.ts

# 3. Let's search for potential instantiations with different patterns
rg "ConstructorObject\(" -A 2

Length of output: 1756


Script:

#!/bin/bash
# Let's check the test file that references ConstructorObject to see how it's used
cat core/core-decorator/test/decorators.test.ts

# Also check the other file that mentions it
cat core/common-util/test/ObjectUtil.test.ts

Length of output: 11840

core/core-decorator/src/decorator/Inject.ts (2)

22-27: Ensure Custom Qualifiers Are Not Overridden

In the logic where you're assigning initType, if a custom initializer type (customInitType) exists, the default info.initType should not override it. Currently, initType is only assigned when customInitType is falsy. Double-check that this aligns with the intended behavior to prevent unintended overrides.


Line range hint 71-99: Validate initType Assignment in constructorInject

Similar to propertyInject, verify that initType is correctly assigned in constructorInject. Ensure that when injectParam is present, it doesn't conflict with or overwrite the initType inferred by guessInjectInfo.

Copy link
Contributor

@killagu killagu left a comment

Choose a reason for hiding this comment

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

LGTM

@gxkl gxkl merged commit 538ae80 into master Oct 30, 2024
12 checks passed
@gxkl gxkl deleted the feat/default-inject-init-type branch October 30, 2024 03:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants