fix(di): fix intercept stored token on context.target #2940
Conversation
WalkthroughThe pull request introduces enhancements to the 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
packages/di/src/common/decorators/intercept.spec.ts(2 hunks)packages/di/src/common/decorators/intercept.ts(1 hunks)
🔇 Additional comments (2)
packages/di/src/common/decorators/intercept.ts (1)
32-32: LGTM: Correct fix for context.target reference
The change from target to klass is the right fix as it ensures the interceptor context receives the actual class constructor instead of the prototype. This aligns with the expected behavior where interceptors should have access to the service class itself.
packages/di/src/common/decorators/intercept.spec.ts (1)
Line range hint 32-32: Verify other usages of context.target
Let's verify that this change doesn't affect other parts of the codebase that might rely on context.target.
✅ Verification successful
Based on the search results, I can now generate the final response:
No breaking changes detected in context.target usage
The search results show that context.target is used consistently across the codebase:
- In documentation examples for error handling
- In platform cache interceptor for:
- Endpoint checks
- Logging
- Cache key generation
- In the test file itself for name verification
All these usages are compatible with the current implementation and don't show any potential issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of context.target to ensure compatibility
rg -A 3 "context\.target" --type ts
Length of output: 2085
| @@ -13,6 +13,8 @@ class MyInterceptor implements InterceptorMethods { | |||
| const r = typeof context.args[0] === "string" ? undefined : new Error(`Error message`); | |||
| const retValue = context.next(r); | |||
|
|
|||
| expect(nameOf(context.target)).toContain("Service"); | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider a more precise assertion for context.target
While checking for "Service" works, it would be more robust to verify the exact class name matches the expected service class.
Consider updating the assertion to be more specific:
-expect(nameOf(context.target)).toContain("Service");
+expect(nameOf(context.target)).toBe("ServiceTest");Committable suggestion skipped: line range outside the PR's diff.
|
🎉 This PR is included in version 8.3.4 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Summary by CodeRabbit
New Features
MyInterceptorclass to verify context identification.Bug Fixes
Tests