fix(platform-http): doesn't call inject.get when provider is instance… #2937
Conversation
WalkthroughThe changes involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Injector
participant Provider
User->>Injector: Request to create injector
Injector->>Provider: Check DEFAULT_PROVIDERS
alt Exclude PlatformResponse and PlatformRequest
Injector->>Provider: Filter out excluded providers
end
Injector->>Provider: Retrieve instances of remaining providers
Injector-->>User: Return injector instance
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
packages/platform/platform-http/src/common/utils/createInjector.ts(2 hunks)
🔇 Additional comments (2)
packages/platform/platform-http/src/common/utils/createInjector.ts (2)
2-2: LGTM: Import addition is necessary and well-placed
The addition of ProviderScope import is required for the new scope checking functionality.
52-54: Verify platform initialization for non-singleton providers
The change to only initialize singleton-scoped providers is a significant behavioral change. Please verify:
- That non-singleton providers don't require initialization during injector creation
- That this change doesn't break any platform features that depend on immediate provider initialization
✅ Verification successful
Let me gather more information about the platform initialization and provider scopes.
Based on the gathered information, I can now provide a final response:
The singleton-only initialization is correct and safe
The codebase analysis reveals that non-singleton providers (PlatformRequest and PlatformResponse) are explicitly scoped as INSTANCE, meaning they are designed to be created per request/response cycle, not during platform initialization. The remaining DEFAULT_PROVIDERS (Platform, PlatformHandler, PlatformApplication) are all explicitly marked as SINGLETON scope.
- PlatformRequest: ProviderScope.INSTANCE
- PlatformResponse: ProviderScope.INSTANCE
- Platform: ProviderScope.SINGLETON
- PlatformHandler: ProviderScope.SINGLETON
- PlatformApplication: ProviderScope.SINGLETON
The change to only initialize singleton providers during injector creation is correct as instance-scoped providers should be created on-demand during request handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any tests or documentation that might indicate required initialization order
rg -A 5 "DEFAULT_PROVIDERS" packages/platform/platform-http/src/
# Look for potential dependencies on immediate initialization
ast-grep --pattern 'class $_ implements PlatformHandler {
$$$
constructor($$$) {
$$$
}
$$$
}'
Length of output: 1313
Script:
#!/bin/bash
# Search for provider scope definitions and usage
rg -A 5 "ProviderScope" packages/platform/platform-http/src/
# Look for platform initialization tests
rg -A 10 "describe.*Platform" packages/platform/platform-http/test/
# Search for any documentation about provider scopes
fd README.md packages/platform/platform-http/ --exec cat {}
Length of output: 12747
| DEFAULT_PROVIDERS.map((provider) => { | ||
| return inj.getProvider(provider.token)?.scope === ProviderScope.SINGLETON && inj.get(provider.token); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace map with forEach since the return value is unused
The current implementation uses map but discards the return value. Since we're only interested in the side effects, forEach would be more appropriate.
- DEFAULT_PROVIDERS.map((provider) => {
+ DEFAULT_PROVIDERS.forEach((provider) => {
return inj.getProvider(provider.token)?.scope === ProviderScope.SINGLETON && inj.get(provider.token);
});📝 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.
| DEFAULT_PROVIDERS.map((provider) => { | |
| return inj.getProvider(provider.token)?.scope === ProviderScope.SINGLETON && inj.get(provider.token); | |
| }); | |
| DEFAULT_PROVIDERS.forEach((provider) => { | |
| return inj.getProvider(provider.token)?.scope === ProviderScope.SINGLETON && inj.get(provider.token); | |
| }); |
Romakita
force-pushed
the
fix-platform-builder-bootstrap
branch
from
516986e to
29dfb36
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
packages/platform/platform-http/src/common/utils/createInjector.ts(1 hunks)
🔇 Additional comments (2)
packages/platform/platform-http/src/common/utils/createInjector.ts (2)
52-54: Replace map with forEach since the return value is unused
The current implementation uses map but discards the return value. Since we're only interested in the side effects, forEach would be more appropriate.
- DEFAULT_PROVIDERS.filter((provider) => ![PlatformResponse, PlatformRequest].includes(provider.token as never)).map((provider) => {
- return inj.get(provider.token);
- });
+ DEFAULT_PROVIDERS.filter((provider) => ![PlatformResponse, PlatformRequest].includes(provider.token)).forEach((provider) => {
+ inj.get(provider.token);
+ });52-54: 🧹 Nitpick (assertive)
Verify the impact of delayed PlatformResponse and PlatformRequest instantiation
The change to filter out PlatformResponse and PlatformRequest from eager instantiation represents a significant behavioral change. This could affect the initialization timing of these services and any dependents.
Let's verify the impact:
✅ Verification successful
Based on the search results, I can now generate the final response since I have enough information to verify the impact of delayed PlatformResponse and PlatformRequest instantiation:
Delayed instantiation of PlatformResponse and PlatformRequest is safe
The change to filter out PlatformResponse and PlatformRequest from eager instantiation is safe because:
- Both classes are designed to be instance-scoped (using
ProviderScope.INSTANCE), meaning they are meant to be created per-request. - They are properly instantiated during context creation in
PlatformContext.ts:this.response = new (options.ResponseKlass || PlatformResponse)(this); this.request = new (options.RequestKlass || PlatformRequest)(this);
- All usages in the codebase are through dependency injection in request handlers or after context creation, with no evidence of early initialization requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for places where PlatformResponse or PlatformRequest are used
# to ensure they don't require eager initialization
# Check for direct dependencies on these services
ast-grep --pattern 'class $_ {
constructor($$$, private $service: PlatformResponse, $$$) {
$$$
}
}'
ast-grep --pattern 'class $_ {
constructor($$$, private $service: PlatformRequest, $$$) {
$$$
}
}'
# Check for early usage in the application lifecycle
rg -A 5 "PlatformResponse|PlatformRequest" --type ts
Length of output: 55520
| @@ -49,7 +49,9 @@ export function createInjector(settings: Partial<TsED.Configuration>) { | |||
| inj.addProvider(token, provider); | |||
| }); | |||
|
|
|||
| DEFAULT_PROVIDERS.map((provider) => inj.get(provider.token)); | |||
| DEFAULT_PROVIDERS.filter((provider) => ![PlatformResponse, PlatformRequest].includes(provider.token as never)).map((provider) => { | |||
There was a problem hiding this comment.
Remove unsafe type assertion
The as never type assertion is a code smell that might hide potential type issues. Consider using proper typing:
- DEFAULT_PROVIDERS.filter((provider) => ![PlatformResponse, PlatformRequest].includes(provider.token as never))
+ DEFAULT_PROVIDERS.filter((provider) => ![PlatformResponse, PlatformRequest].includes(provider.token))Committable suggestion skipped: line range outside the PR's diff.
|
🎉 This PR is included in version 8.3.3 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Summary by CodeRabbit
New Features
Bug Fixes