fix(platform-http): update the adapter configuration to be aligned with DI convention #2999
Conversation
|
Warning Rate limit exceeded@Romakita has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 40 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (16)
WalkthroughThe pull request updates dependency injection and adapter configurations across the platform modules. It removes broad providers arrays in favor of more targeted adapter function calls and modifies function signatures to accept dependency arrays. The changes affect how middleware, application, adapter, and handler classes interact, leading to a streamlined injection process. Test cases are updated to reflect the new injection strategies, and obsolete methods related to multipart handling are removed. Changes
Possibly related PRs
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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
packages/platform/platform-express/src/components/PlatformExpress.ts(1 hunks)packages/platform/platform-http/src/common/fn/adapter.ts(1 hunks)packages/platform/platform-http/src/common/middlewares/PlatformMulterMiddleware.spec.ts(3 hunks)packages/platform/platform-http/src/common/middlewares/PlatformMulterMiddleware.ts(2 hunks)packages/platform/platform-http/src/common/services/PlatformAdapter.ts(3 hunks)packages/platform/platform-http/src/common/services/PlatformApplication.spec.ts(0 hunks)packages/platform/platform-http/src/common/services/PlatformApplication.ts(2 hunks)packages/platform/platform-http/src/common/services/PlatformHandler.ts(2 hunks)packages/platform/platform-http/src/common/services/PlatformMiddlewaresChain.ts(1 hunks)packages/platform/platform-http/src/common/utils/createInjector.ts(2 hunks)packages/platform/platform-koa/src/components/PlatformKoa.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/platform/platform-http/src/common/services/PlatformApplication.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: test-third-parties (20.12.2)
- GitHub Check: test-specs (20.12.2)
- GitHub Check: test-security (20.12.2)
- GitHub Check: test-platform (20.12.2)
- GitHub Check: test-orm (20.12.2)
- GitHub Check: test-core (20.12.2)
- GitHub Check: test-graphql (20.12.2)
- GitHub Check: CodeQL-Build
🔇 Additional comments (19)
packages/platform/platform-http/src/common/services/PlatformHandler.ts (1)
18-18: New import for PlatformAdapter.
This import aligns with the new injection mechanism for the adapter. No issues identified.packages/platform/platform-http/src/common/services/PlatformMiddlewaresChain.ts (1)
2-2: Removal of unused imports.
Removing theinjectimport in favor of only importing what is used (constant,injectable) simplifies the code. Looks good.packages/platform/platform-http/src/common/middlewares/PlatformMulterMiddleware.ts (1)
9-9: Import of PlatformAdapter.
This import is part of the shift fromPlatformApplicationtoPlatformAdapter. No specific issues found.packages/platform/platform-http/src/common/utils/createInjector.ts (3)
1-1: Updated import for logger configuration.
Narrowed imports to only what's needed. No issues identified.
20-20: Side-effect call to $adapter.
Ensure that$adapter(settings.adapter)is only needed for its side effects and that discarding its return value is intentional.
22-22: Invoking PlatformAdapter.
inj.invoke(PlatformAdapter)properly initializes the adapter. Confirm that the adapter is configured as expected and no references are required afterward.packages/platform/platform-koa/src/components/PlatformKoa.ts (1)
189-202: Refactored DI approachThis newly introduced array approach for injecting providers is consistent with the new adapter signature. Please verify if
PlatformApplicationor any other providers are needed here, otherwise it looks good.packages/platform/platform-express/src/components/PlatformExpress.ts (1)
278-283: Ensure consistency with adapter approachThis configuration adds only the
PlatformHandlerto the adapter. If you need to rely onPlatformRequestorPlatformResponse, consider including them. Otherwise, it looks correct.packages/platform/platform-http/src/common/middlewares/PlatformMulterMiddleware.spec.ts (3)
2-2: New imports for injectionThe direct imports of
injectfrom "@tsed/di" andPlatformAdapterare a good step toward a more streamlined approach to dependency injection.Also applies to: 10-10
25-25: Refactoring test constructionInjecting
PlatformMulterMiddlewareandPlatformAdapterdirectly clarifies dependencies and reduces reliance on global mocks. Good improvement.Also applies to: 29-29, 31-31, 33-33
46-46: Updated test usage of adapter.multipartReplacing
app.multerreferences withadapter.multipartensures that the new approach is properly tested. This looks correct and aligned with the updated architecture.Also applies to: 50-50, 57-57, 63-63
packages/platform/platform-http/src/common/fn/adapter.ts (2)
2-2: Imports updated for DI reworkBringing in
ProviderOptsandPlatformAdapteraligns with the new DI approach. This looks correct and necessary.Also applies to: 4-4
11-17: Extended adapter function signatureThe introduction of an optional second argument to configure additional providers is a solid improvement. Ensure concurrency is handled carefully if providers are updated after the adapter is set. Overall, this pattern fosters clarity in dependency registration.
Also applies to: 18-18, 23-30, 31-36
packages/platform/platform-http/src/common/services/PlatformAdapter.ts (5)
4-5: No issues found with the new imports.
These imports correctly pull in additional DI and hook features.
14-16: Clarify usage of newly imported dependencies.
Imports forPlatform,PlatformApplication, andPlatformHandlerwork fine, but ensure that usage in other parts of the code (or re-exports) is properly aligned with the updated DI context.
21-21: Use caution when converting from a getter to a readonly injection.
Previously, this property was a getter. Now it is a readonly field injected at construction time. While this is generally acceptable, confirm that existing references tothis.app(which might have expected lazy evaluation) still function as intended.
23-33: Ensure that the constructor injection does not introduce circular references.
InjectingPlatformApplicationwithin thePlatformAdapterconstructor is straightforward, but double-check that other parts of the application will not cause circular injection or timing issues (e.g., ifPlatformApplicationalso injects adapters at some point).Also note that
$on("$afterInvoke", PlatformAdapter, ...)is triggered after this class is invoked, settingPLATFORM_NAMEin the configuration. Please confirm that there is no duplication or conflict with other modules that might also overridePLATFORM_NAME.
99-99: Good practice to explicitly declare imports in the DI metadata.
Declaring[PlatformApplication, Platform, PlatformHandler]asimportsfor this adapter is clear and helps avoid hidden dependency issues.packages/platform/platform-http/src/common/services/PlatformApplication.ts (1)
3-3: Scope declaration is consistent with the single instance usage.
IntroducingProviderScope.SINGLETONis appropriate for an application-level service. Make sure this aligns with how the rest of the platform expects to interact withPlatformApplication.
| @@ -43,7 +42,7 @@ export class PlatformHandler { | |||
| .on("alterHandler", (handlerMetadata: PlatformHandlerMetadata) => { | |||
| const handler = handlerMetadata.isInjectable() ? this.createHandler(handlerMetadata) : handlerMetadata.handler; | |||
|
|
|||
| return this.platformApplication.adapter.mapHandler(handler, handlerMetadata); | |||
| return inject(PlatformAdapter).mapHandler(handler, handlerMetadata); | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider storing the adapter instance in a property.
Directly calling inject(PlatformAdapter) on each handler mapping might cause minor overhead and makes it less explicit than having a dedicated property injection. Consider injecting once (e.g., in the constructor) to keep the code consistent and potentially more efficient.
| @@ -39,7 +38,7 @@ export class PlatformMulterMiddleware implements MiddlewareMethods { | |||
| delete settings.dest; | |||
| } | |||
|
|
|||
| const multer = this.app.multer(settings); | |||
| const multer = inject(PlatformAdapter).multipart(settings); | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Refactor injection usage.
Using inject(PlatformAdapter).multipart(settings) is correct for dynamically retrieving the adapter. If this call happens frequently, consider storing a reference in the class to avoid repetitive injection.
| this.rawApp = app; | ||
| this.rawCallback = callback; | ||
| } | ||
| rawCallback(): any {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Empty method body may lead to unhandled callbacks.
Previously, rawCallback might have been populated by the adapter; now it is an empty method returning any. Verify if other parts of the platform expect a functional implementation. If so, consider either implementing a default callback or refactoring the call site.
Romakita
force-pushed
the
fix-adapter-injection
branch
from
6a27230 to
f703e46
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
packages/platform/platform-express/src/components/PlatformExpress.ts(1 hunks)packages/platform/platform-express/vitest.config.mts(1 hunks)packages/platform/platform-http/src/common/fn/adapter.ts(1 hunks)packages/platform/platform-http/src/common/middlewares/PlatformMulterMiddleware.spec.ts(3 hunks)packages/platform/platform-http/src/common/middlewares/PlatformMulterMiddleware.ts(2 hunks)packages/platform/platform-http/src/common/services/PlatformAdapter.ts(3 hunks)packages/platform/platform-http/src/common/services/PlatformApplication.spec.ts(0 hunks)packages/platform/platform-http/src/common/services/PlatformApplication.ts(2 hunks)packages/platform/platform-http/src/common/services/PlatformHandler.ts(2 hunks)packages/platform/platform-http/src/common/services/PlatformMiddlewaresChain.ts(1 hunks)packages/platform/platform-http/src/common/utils/createInjector.ts(2 hunks)packages/platform/platform-koa/src/components/PlatformKoa.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/platform/platform-http/src/common/services/PlatformApplication.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: test-third-parties (20.12.2)
- GitHub Check: test-specs (20.12.2)
- GitHub Check: test-security (20.12.2)
- GitHub Check: test-platform (20.12.2)
- GitHub Check: test-orm (20.12.2)
- GitHub Check: test-graphql (20.12.2)
- GitHub Check: test-core (20.12.2)
- GitHub Check: CodeQL-Build
🔇 Additional comments (21)
packages/platform/platform-express/vitest.config.mts (1)
13-16: Consider verifying coverage objectives.You've slightly lowered the coverage thresholds from
96.63to96.61. Please confirm that this change is intentional and won't mask any regressions in test coverage.packages/platform/platform-http/src/common/services/PlatformHandler.ts (2)
18-18: Import forPlatformAdapterlooks correct.This import is properly aligned with the direct injection usage in this file.
45-45: Consider storing the adapter instance in a property.
We've flagged this in an earlier commit. Reiterating the recommendation to store thePlatformAdapterinstance in a class property or constructor injection for consistency and possible performance benefits.packages/platform/platform-http/src/common/services/PlatformMiddlewaresChain.ts (1)
2-2: Removal of unused import is good.Dropping
injectfrom the imports helps keep the file clean.packages/platform/platform-http/src/common/middlewares/PlatformMulterMiddleware.ts (2)
9-9: Import forPlatformAdapterlooks appropriate.This aligns with the direct injection used in the middleware.
41-41: Refactor repeated injection usage.
InjectingPlatformAdapterinline may cause overhead if called frequently. Consider storing the adapter instance in the class to avoid repetitive injection calls.packages/platform/platform-koa/src/components/PlatformKoa.ts (1)
189-202: Adopt the new targeted DI approach carefully.This new
adapter(PlatformKoa, [...])array-based provider registration aligns well with your refactoring goal of explicitly declaring dependencies. Since the oldprovidersproperty was removed, ensure all references to it are replaced or removed accordingly. Also confirm whether additional tokens (e.g.,PlatformApplication,Platform) need registering or are covered by this slimmed-down list.packages/platform/platform-http/src/common/utils/createInjector.ts (2)
1-1: Fresh import usage is aligned with the DI patterns.Importing
injectorandsetLoggerConfigurationfrom@tsed/dilooks correct and consistent with the codebase’s DI design. No immediate concerns here.
20-22: Confirm the adapter call and injector invocation sequence.These combined lines (
$adapter(settings.adapter)followed byinj.invoke(PlatformAdapter)) depend on the adapter’s internal logic. Verify that no other tokens or providers are necessary before invokingPlatformAdapter. If any providers were previously automatically injected, confirm they’re now handled by the new approach.packages/platform/platform-express/src/components/PlatformExpress.ts (1)
278-283: Narrow provider array for PlatformExpress requires verification.Registering only
PlatformHandlerinadapter(PlatformExpress, [...])follows the new DI pattern. However, confirm that you don’t needPlatformRequest,PlatformResponse, or others. If future logic relies on them, they should also be declared here or seamlessly injected elsewhere.packages/platform/platform-http/src/common/services/PlatformAdapter.ts (2)
4-16: Validate import statements
The new imports from@tsed/diand@tsed/hookslook correct. Ensure your environment is configured for ESM builds and that the “.js” extension usage is consistently handled across modules.
99-99: Explicitly declaring imports
Registering these dependencies ensures thePlatformAdapterhas all required services at injection time. This approach aligns well with Ts.ED’s DI patterns.packages/platform/platform-http/src/common/services/PlatformApplication.ts (2)
3-3: Check the provider scope
UsingProviderScope.SINGLETONhelps reuse a single instance. However, confirm that a shared instance ofPlatformApplicationdoes not introduce side effects when handling multiple requests or distinct contexts.
21-21: MethodrawCallback()is empty
This repeats the prior concern: an empty implementation might lead to unhandled callbacks if other parts of the platform assume a functional method.packages/platform/platform-http/src/common/middlewares/PlatformMulterMiddleware.spec.ts (7)
2-2: Use ofinjectis aligned with the new DI approach.
This import correctly introduces theinjectfunction from@tsed/difor a streamlined dependency injection workflow.
10-10: NewPlatformAdapterimport is consistent with DI refactor.
ImportingPlatformAdapterhere ensures it can be directly injected and tested, removing reliance on a mockappobject.
25-33: Direct injection ofPlatformMulterMiddlewareandPlatformAdapteris a clean refactor.
ReplacingPlatformTest.invoke(...)withinject(...)removes boilerplate while maintaining test clarity. The return structure now includesadapter, avoiding the previous mockapp.
46-46: Destructuringadapterin test setup.
Includingadapterin the returned object aligns with the updated dependency injection flow, ensuring consistent usage in tests.
50-50: Replacingapp.multerwithadapter.multipartin assertions.
This change correctly tests the new adapter-based approach to file handling, reflecting the updated middleware logic.
57-57: Storage test aligned with adapter injection.
The refactoring to rely onadapter.multipartwith astorageparameter is coherent with the new design.
63-63: Adapter-based assertion formultipartcall.
Confirming that theadapter.multipartmethod is called with the correct options ensures correctness when using a custom storage.
| import {GlobalProviders, injector, ProviderOpts, refValue} from "@tsed/di"; | ||
|
|
||
| import type {PlatformAdapter} from "../services/PlatformAdapter.js"; | ||
| import {PlatformAdapter} from "../services/PlatformAdapter.js"; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use consistent ESM extension
The import statements with ".js" extension are valid for ESM usage. Just ensure your bundler or environment supports it consistently across the platform.
| * Set the platform adapter token and his dependencies | ||
| */ | ||
| export function adapter(adapter?: Type<PlatformAdapter<any>>): Type<PlatformAdapter<any>> { | ||
| export function adapter(adapter: Type<PlatformAdapter<any>>, imports?: ProviderOpts[]): Type<PlatformAdapter<any>>; | ||
| /** | ||
| * Get the platform adapter token | ||
| */ | ||
| export function adapter(): Type<PlatformAdapter<any>>; | ||
| export function adapter(adapter?: Type<PlatformAdapter<any>>, imports: ProviderOpts[] = []): Type<PlatformAdapter<any>> { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clarify function overload documentation
The JSDoc references “Set the platform adapter token and his dependencies,” but doesn’t address the overload that retrieves the adapter token. Consider updating or separating the doc blocks to reflect both usage scenarios.
| imports?.forEach(({token, useClass}) => { | ||
| const provider = GlobalProviders.get(token); | ||
| if (useClass && provider) { | ||
| provider.useClass = useClass; | ||
| injector().set(token, provider); | ||
| } | ||
| }); | ||
|
|
||
| injector() | ||
| .addProvider(PlatformAdapter, { | ||
| useClass: adapter | ||
| }) | ||
| .alias(PlatformAdapter, "PlatformAdapter"); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Handle missing providers and possible concurrency side effects
When reassigning provider.useClass in the loop, ensure that GlobalProviders.get(token) is defined. Otherwise, a missing provider might cause runtime errors. Also verify if reassigning useClass could lead to concurrency or lifecycle issues if multiple calls to adapter() modify the same provider in parallel.
| readonly app = inject<PlatformApplication<App>>(PlatformApplication); | ||
|
|
||
| constructor() { | ||
| const platformApp = inject(PlatformApplication); | ||
|
|
||
| const {app, callback} = this.createApp(); | ||
| platformApp.rawApp = app as any; | ||
| platformApp.rawCallback = callback; | ||
|
|
||
| $on("$afterInvoke", PlatformAdapter, () => { | ||
| configuration().set("PLATFORM_NAME", constant("PLATFORM_NAME") || this.NAME); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Unify usage of injected references and confirm $afterInvoke usage
The constructor re-injects PlatformApplication instead of relying on this.app. You could reference this.app to avoid redundancy. Also verify that $afterInvoke is the desired lifecycle hook for finalizing the PLATFORM_NAME; depending on your application flow, you might need an earlier or later hook.
Romakita
force-pushed
the
fix-adapter-injection
branch
from
f703e46 to
35071a7
Compare
Romakita
force-pushed
the
fix-adapter-injection
branch
from
814590c to
43fe027
Compare
|
🎉 This PR is included in version 8.5.2 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Information
Refactor how PlatformAdapter is built. Now, PlatformAdapter declare correctly his dependencies and configure PlatformApplication. It avoid issue if the PlatformAdapter is instanciated in another DI context (CLI DI context).
Summary by CodeRabbit
Refactor
PlatformApplicationandPlatformHandlerclasses by removing unnecessary dependencies onPlatformAdapter.PlatformKoaandPlatformExpressclasses to refine provider registration.New Features
Tests