-
Notifications
You must be signed in to change notification settings - Fork 12k
fix: enable DI for FeatureOptInService #26061
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
a419f7b
fix integration test
eunjae-lee 20cbcf3
fix: enable DI for FeatureOptInService
eunjae-lee 62f8176
create containers/FeaturesRepository.ts
eunjae-lee 470d2c0
refactor: convert FeatureOptInService and FeaturesRepository to modul…
devin-ai-integration[bot] 4773edb
refactor: rename FeatureOptInServiceInterface to IFeatureOptInService
devin-ai-integration[bot] 0a89187
Merge branch 'main' into fix/di-feature-opt-in-service
eunjae-lee 259569f
fix: export featuresRepositoryModule for backward compatibility
devin-ai-integration[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| import type { IFeatureOptInService } from "@calcom/features/feature-opt-in/services/IFeatureOptInService"; | ||
|
|
||
| import { createContainer } from "../di"; | ||
| import { moduleLoader as featureOptInServiceModuleLoader } from "../modules/FeatureOptInService"; | ||
|
|
||
| const featureOptInServiceContainer = createContainer(); | ||
|
|
||
| export function getFeatureOptInService(): IFeatureOptInService { | ||
| featureOptInServiceModuleLoader.loadModule(featureOptInServiceContainer); | ||
| return featureOptInServiceContainer.get<IFeatureOptInService>(featureOptInServiceModuleLoader.token); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| import { createContainer } from "../di"; | ||
| import { type FeaturesRepository, moduleLoader as featuresRepositoryModuleLoader } from "../modules/FeaturesRepository"; | ||
|
|
||
| const featuresRepositoryContainer = createContainer(); | ||
|
|
||
| export function getFeaturesRepository(): FeaturesRepository { | ||
| featuresRepositoryModuleLoader.loadModule(featuresRepositoryContainer); | ||
| return featuresRepositoryContainer.get<FeaturesRepository>(featuresRepositoryModuleLoader.token); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| import { FEATURE_OPT_IN_DI_TOKENS } from "@calcom/features/feature-opt-in/di/tokens"; | ||
| import { FeatureOptInService } from "@calcom/features/feature-opt-in/services/FeatureOptInService"; | ||
|
|
||
| import { bindModuleToClassOnToken, createModule, type ModuleLoader } from "../di"; | ||
| import { moduleLoader as featuresRepositoryModuleLoader } from "./FeaturesRepository"; | ||
|
|
||
| const thisModule = createModule(); | ||
| const token = FEATURE_OPT_IN_DI_TOKENS.FEATURE_OPT_IN_SERVICE; | ||
| const moduleToken = FEATURE_OPT_IN_DI_TOKENS.FEATURE_OPT_IN_SERVICE_MODULE; | ||
|
|
||
| const loadModule = bindModuleToClassOnToken({ | ||
| module: thisModule, | ||
| moduleToken, | ||
| token, | ||
| classs: FeatureOptInService, | ||
| dep: featuresRepositoryModuleLoader, | ||
| }); | ||
|
|
||
| export const moduleLoader: ModuleLoader = { | ||
| token, | ||
| loadModule, | ||
| }; | ||
|
|
||
| export type { FeatureOptInService }; |
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| import { FLAGS_DI_TOKENS } from "@calcom/features/flags/di/tokens"; | ||
| import { FeaturesRepository } from "@calcom/features/flags/features.repository"; | ||
|
|
||
| import { bindModuleToClassOnToken, createModule, type ModuleLoader } from "../di"; | ||
| import { moduleLoader as prismaModuleLoader } from "./Prisma"; | ||
|
|
||
| export const featuresRepositoryModule = createModule(); | ||
| const token = FLAGS_DI_TOKENS.FEATURES_REPOSITORY; | ||
| const moduleToken = FLAGS_DI_TOKENS.FEATURES_REPOSITORY_MODULE; | ||
|
|
||
| const loadModule = bindModuleToClassOnToken({ | ||
| module: featuresRepositoryModule, | ||
| moduleToken, | ||
| token, | ||
| classs: FeaturesRepository, | ||
| dep: prismaModuleLoader, | ||
| }); | ||
|
|
||
| export const moduleLoader: ModuleLoader = { | ||
| token, | ||
| loadModule, | ||
| }; | ||
|
|
||
| export type { FeaturesRepository }; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| export const FEATURE_OPT_IN_DI_TOKENS = { | ||
| FEATURE_OPT_IN_SERVICE: Symbol("FeatureOptInService"), | ||
| FEATURE_OPT_IN_SERVICE_MODULE: Symbol("FeatureOptInServiceModule"), | ||
| }; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
39 changes: 39 additions & 0 deletions
39
packages/features/feature-opt-in/services/IFeatureOptInService.ts
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I think the convention is to name interface files starting with I |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| import type { FeatureId, FeatureState } from "@calcom/features/flags/config"; | ||
|
|
||
| export type ResolvedFeatureState = { | ||
| featureId: FeatureId; | ||
| globalEnabled: boolean; | ||
| orgState: FeatureState; // Raw state (before auto-opt-in transform) | ||
| teamStates: FeatureState[]; // Raw states | ||
| userState: FeatureState | undefined; // Raw state | ||
| effectiveEnabled: boolean; | ||
| // Auto-opt-in flags for UI to show checkbox state | ||
| orgAutoOptIn: boolean; | ||
| teamAutoOptIns: boolean[]; | ||
| userAutoOptIn: boolean; | ||
| }; | ||
|
|
||
| export interface IFeatureOptInService { | ||
| resolveFeatureStatesAcrossTeams(input: { | ||
| userId: number; | ||
| orgId: number | null; | ||
| teamIds: number[]; | ||
| featureIds: FeatureId[]; | ||
| }): Promise<Record<string, ResolvedFeatureState>>; | ||
| listFeaturesForUser(input: { userId: number; orgId: number | null; teamIds: number[] }): Promise< | ||
| ResolvedFeatureState[] | ||
| >; | ||
| listFeaturesForTeam( | ||
| input: { teamId: number } | ||
| ): Promise<{ featureId: FeatureId; globalEnabled: boolean; teamState: FeatureState }[]>; | ||
| setUserFeatureState( | ||
| input: | ||
| | { userId: number; featureId: FeatureId; state: "enabled" | "disabled"; assignedBy: number } | ||
| | { userId: number; featureId: FeatureId; state: "inherit" } | ||
| ): Promise<void>; | ||
| setTeamFeatureState( | ||
| input: | ||
| | { teamId: number; featureId: FeatureId; state: "enabled" | "disabled"; assignedBy: number } | ||
| | { teamId: number; featureId: FeatureId; state: "inherit" } | ||
| ): Promise<void>; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| export const FLAGS_DI_TOKENS = { | ||
| FEATURES_REPOSITORY: Symbol("FeaturesRepository"), | ||
| FEATURES_REPOSITORY_MODULE: Symbol("FeaturesRepositoryModule"), | ||
| }; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Potential test isolation issue:
getFeaturesRepository()andgetFeatureOptInService()use separate DI containers, creating differentFeaturesRepositoryinstances. TheclearFeaturesCache(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:
Prompt for AI agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review! However, I believe this concern is not actually valid for this specific case.
The
FeaturesRepositorycache is a static property (class-level, not instance-level):And
clearCache()clears this static property:Since the cache is static, calling
clearFeaturesCache(featuresRepository)clears the cache for all instances ofFeaturesRepository, 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.