fix: enable DI for FeatureOptInService#26061
Conversation
github-actions
bot
added
❗️ migrations
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
E2E results are ready! |
eunjae-lee
force-pushed
the
fix/di-feature-opt-in-service
branch
from
6fa75c8 to
e4288a9
Compare
eunjae-lee
force-pushed
the
feat/feature-opt-in-service
branch
from
bc58bb0 to
d075b06
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. |
eunjae-lee
changed the base branch from
feat/feature-opt-in-service
to
graphite-base/26061
|
This PR has been marked as stale due to inactivity. If you're still working on it or need any help, please let us know or update the PR to keep it active. |
eunjae-lee
force-pushed
the
fix/di-feature-opt-in-service
branch
from
1ca81e1 to
62f8176
Compare
| const container = createContainer(); | ||
| container.load(DI_TOKENS.PRISMA_MODULE, prismaModule); | ||
| container.load(DI_TOKENS.FEATURES_REPOSITORY_MODULE, featuresRepositoryModule); | ||
| container.load(DI_TOKENS.FEATURE_OPT_IN_SERVICE_MODULE, featureOptInServiceModule); |
There was a problem hiding this comment.
This approach to load dependencies isn't build time safe. We might endup in a scenario, where FeaturesOptinService adds a new dependency and we forget to load it here and TS will happily pass.
We should instead use the moduleLoader pattern used in many places now. You would need to convert FeaturesOptInService module to that pattern and then TS would know what dependences are needed by it and would automatically loaded them as well.
| import { createContainer } from "../di"; | ||
| import { featuresRepositoryModule } from "../modules/FeaturesRepository"; | ||
|
|
||
| const container = createContainer(); | ||
| container.load(DI_TOKENS.PRISMA_MODULE, prismaModule); | ||
| container.load(DI_TOKENS.FEATURES_REPOSITORY_MODULE, featuresRepositoryModule); | ||
|
|
||
| export function getFeaturesRepository() { |
There was a problem hiding this comment.
We can convert it to use moduleLoader format too
packages/features/di/tokens.ts
Outdated
| FEATURE_OPT_IN_SERVICE: Symbol("FeatureOptInService"), | ||
| FEATURE_OPT_IN_SERVICE_MODULE: Symbol("FeatureOptInServiceModule"), |
There was a problem hiding this comment.
The convention is to expose tokens.ts file from the feature package itself and just impor tthat here.
There was a problem hiding this comment.
nit: I think the convention is to name interface files starting with I
…eLoader pattern - Create feature-specific tokens in feature-opt-in/di/tokens.ts and flags/di/tokens.ts - Update modules to use bindModuleToClassOnToken for type-safe dependency injection - Simplify containers to use moduleLoader.loadModule() for automatic dependency loading - Import feature-specific tokens in central tokens.ts using spread operator Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
Follow the codebase convention of using 'I' prefix for interface files and names. Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
The FeaturesRepository module was refactored to use moduleLoader pattern but AvailableSlots.ts still imports featuresRepositoryModule. This adds the export to maintain backward compatibility. Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
|
@hariombalhara addressed your feedback ! |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 15 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/features/feature-opt-in/services/FeatureOptInService.integration-test.ts">
<violation number="1" location="packages/features/feature-opt-in/services/FeatureOptInService.integration-test.ts:82">
P2: Potential test isolation issue: `getFeaturesRepository()` and `getFeatureOptInService()` use separate DI containers, creating different `FeaturesRepository` instances. The `clearFeaturesCache(featuresRepository)` call only clears the cache on the standalone repository instance, not the one used internally by the service. This could cause test flakiness if the service's internal repository has cached stale data.
Consider either:
1. Using a shared DI container for both
2. Exposing a method to get the repository instance from the service for cache clearing
3. Creating a helper that clears both caches</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| const featuresRepository = new FeaturesRepository(prisma); | ||
| const service = new FeatureOptInService(featuresRepository); | ||
| const featuresRepository = getFeaturesRepository(); |
There was a problem hiding this comment.
P2: Potential test isolation issue: getFeaturesRepository() and getFeatureOptInService() use separate DI containers, creating different FeaturesRepository instances. The clearFeaturesCache(featuresRepository) call only clears the cache on the standalone repository instance, not the one used internally by the service. This could cause test flakiness if the service's internal repository has cached stale data.
Consider either:
- Using a shared DI container for both
- Exposing a method to get the repository instance from the service for cache clearing
- Creating a helper that clears both caches
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/feature-opt-in/services/FeatureOptInService.integration-test.ts, line 82:
<comment>Potential test isolation issue: `getFeaturesRepository()` and `getFeatureOptInService()` use separate DI containers, creating different `FeaturesRepository` instances. The `clearFeaturesCache(featuresRepository)` call only clears the cache on the standalone repository instance, not the one used internally by the service. This could cause test flakiness if the service's internal repository has cached stale data.
Consider either:
1. Using a shared DI container for both
2. Exposing a method to get the repository instance from the service for cache clearing
3. Creating a helper that clears both caches</comment>
<file context>
@@ -77,8 +79,8 @@ async function setup(): Promise<TestEntities> {
- const featuresRepository = new FeaturesRepository(prisma);
- const service = new FeatureOptInService(featuresRepository);
+ const featuresRepository = getFeaturesRepository();
+ const service = getFeatureOptInService();
</file context>
There was a problem hiding this comment.
Thanks for the review! However, I believe this concern is not actually valid for this specific case.
The FeaturesRepository cache is a static property (class-level, not instance-level):
private static featuresCache: { data: any[]; expiry: number } | null = null;And clearCache() clears this static property:
private clearCache() {
FeaturesRepository.featuresCache = null;
}Since the cache is static, calling clearFeaturesCache(featuresRepository) clears the cache for all instances of FeaturesRepository, including the one inside the service. The tests work correctly as-is because the cache is shared at the class level, not the instance level.
Additionally, there's a follow-up PR coming that will replace this static cache with a proper Redis cache, which will make this even cleaner.
Devin AI is addressing Cubic AI's review feedbackNew feedback has been sent to the existing Devin session. |
2 participants
What does this PR do?
This PR adds DI support for FeatureOptInService and FeaturesRepository.
But we're still using FeaturesRepository directly across the project, and I will fix it in a separate PR.
Updates since last revision
FeatureOptInServiceInterface.tstoIFeatureOptInService.tsto follow the codebase convention of usingIprefix for interface filesFeatureOptInServiceInterfacetoIFeatureOptInServiceMandatory Tasks (DO NOT REMOVE)
How should this be tested?
yarn type-check:ci --forceTZ=UTC yarn testSummary by cubic
Enables DI for FeatureOptInService and FeaturesRepository across the app. Replaces direct instantiation and Prisma usage with moduleLoader-based containers and updates router/tests to resolve the service via DI.
Written for commit 259569f. Summary will update on new commits.
Link to Devin run: https://app.devin.ai/sessions/de393c9ea2b8412587a8c4ca8cb2631d
Requested by: @eunjae-lee (eunjae@cal.com)