-
Notifications
You must be signed in to change notification settings - Fork 16
Refactor/modules implementation #469
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
base: develop
Are you sure you want to change the base?
Changes from all commits
a925dd4
75c38c8
751fa10
83ddb24
c0e24f6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -98,6 +98,11 @@ export type ModulesConfig<Modules extends ModulesRecord> = { | |
| : never; | ||
| }; | ||
|
|
||
| export type CombinedModuleContainerConfig< | ||
| Modules extends ModulesRecord, | ||
| OwnConfig = NoConfig, | ||
| > = OwnConfig & ModulesConfig<Modules>; | ||
|
|
||
| /** | ||
| * This type make any config partial (i.e. optional) up to the first level | ||
| * So { Module: { a: { b: string } } } | ||
|
|
@@ -139,9 +144,16 @@ export interface ModuleContainerLike { | |
| /** | ||
| * Reusable module container facilitating registration, resolution | ||
| * configuration, decoration and validation of modules | ||
| * | ||
| * @typeParam Modules - The record of child module classes. | ||
| * @typeParam OwnConfig - Optional config type for keys that belong to the | ||
| * container itself (not forwarded to child modules). Defaults to NoConfig. | ||
| */ | ||
| export class ModuleContainer<Modules extends ModulesRecord> | ||
| extends ConfigurableModule<ModulesConfig<Modules>> | ||
| export class ModuleContainer< | ||
| Modules extends ModulesRecord, | ||
| OwnConfig = NoConfig, | ||
| > | ||
| extends ConfigurableModule<CombinedModuleContainerConfig<Modules, OwnConfig>> | ||
| implements ModuleContainerLike | ||
| { | ||
| /** | ||
|
|
@@ -155,10 +167,28 @@ export class ModuleContainer<Modules extends ModulesRecord> | |
|
|
||
| private eventEmitterProxy: EventEmitterProxy<Modules> | undefined = undefined; | ||
|
|
||
| /** | ||
| * Set of config key names that belong to the container | ||
| */ | ||
| protected readonly ownConfigKeys: Set<string> = new Set(); | ||
|
|
||
| public constructor(public definition: Modules) { | ||
| super(); | ||
| } | ||
|
|
||
| /** | ||
| * Returns an object containing the keys listed in ownConfigKeys. | ||
| */ | ||
| public get ownConfig(): OwnConfig { | ||
|
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. In general, I think this is good, a few notes:
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. Also let's rename ownConfig to containerConfig maybe? Or is that too misleading? |
||
| const fullConfig = this.config; | ||
| const result: Record<string, unknown> = {}; | ||
| for (const key of this.ownConfigKeys) { | ||
| result[key] = fullConfig[key]; | ||
| } | ||
| // eslint-disable-next-line @typescript-eslint/consistent-type-assertions | ||
| return result as OwnConfig; | ||
| } | ||
|
|
||
| /** | ||
| * @returns list of module names | ||
| */ | ||
|
|
@@ -296,25 +326,28 @@ export class ModuleContainer<Modules extends ModulesRecord> | |
| * before the first resolution. | ||
| * @param config | ||
| */ | ||
| public configure(config: ModulesConfig<Modules>) { | ||
| public configure(config: CombinedModuleContainerConfig<Modules, OwnConfig>) { | ||
| this.config = config; | ||
| } | ||
|
|
||
| public configurePartial(config: RecursivePartial<ModulesConfig<Modules>>) { | ||
| this.config = merge< | ||
| ModulesConfig<Modules> | NoConfig, | ||
| RecursivePartial<ModulesConfig<Modules>> | ||
| >(this.currentConfig ?? {}, config); | ||
| public configurePartial( | ||
| config: RecursivePartial<CombinedModuleContainerConfig<Modules, OwnConfig>> | ||
| ) { | ||
| // eslint-disable-next-line @typescript-eslint/consistent-type-assertions | ||
| this.config = merge( | ||
| this.currentConfig ?? {}, | ||
| config | ||
| ) as CombinedModuleContainerConfig<Modules, OwnConfig>; | ||
| } | ||
|
|
||
| public get config() { | ||
| return super.config; | ||
| } | ||
|
|
||
| public set config(config: ModulesConfig<Modules>) { | ||
| public set config(config: CombinedModuleContainerConfig<Modules, OwnConfig>) { | ||
| super.config = merge< | ||
| ModulesConfig<Modules> | NoConfig, | ||
| ModulesConfig<Modules> | ||
| CombinedModuleContainerConfig<Modules, OwnConfig> | NoConfig, | ||
| CombinedModuleContainerConfig<Modules, OwnConfig> | ||
| >(this.currentConfig ?? {}, config); | ||
| } | ||
|
|
||
|
|
@@ -365,6 +398,10 @@ export class ModuleContainer<Modules extends ModulesRecord> | |
| moduleName: StringKeyOf<Modules>, | ||
| containedModule: InstanceType<Modules[StringKeyOf<Modules>]> | ||
| ) { | ||
| if (this.ownConfigKeys.has(moduleName)) { | ||
| return; | ||
| } | ||
|
|
||
| const config = super.config?.[moduleName]; | ||
| // eslint-disable-next-line @typescript-eslint/strict-boolean-expressions | ||
| if (!config) { | ||
|
|
||
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.
Just a thought - instead of flattening the Graphql implementation into here. Could we not just make this class a dependencyfactory creating the GraphqlServer? This way users could override it by passing a GraphqlServer module if they don't want to use Koa for some reason