-
Notifications
You must be signed in to change notification settings - Fork 16
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
the modules with controllers were modified to be dynamic #85
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
---|---|---|
|
@@ -13,10 +13,13 @@ export const assistantParams: AssistantCreateParams = { | |
export const assistantConfig: AssistantConfigParams = { | ||
id: process.env['ASSISTANT_ID'] || '', | ||
params: assistantParams, | ||
assistantPrefix: 'assistant-prefix', | ||
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. |
||
filesDir: './apps/api/src/app/knowledge', | ||
toolResources: { | ||
fileSearch: { | ||
fileNames: ['33-things-to-ask-your-digital-product-development-partner.md'], | ||
fileNames: [ | ||
'33-things-to-ask-your-digital-product-development-partner.md', | ||
], | ||
}, | ||
codeInterpreter: { | ||
fileNames: [], | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ import { AgentsModule } from './agents/agents.module'; | |
import { ChatSockets } from './chat.sockets'; | ||
|
||
@Module({ | ||
imports: [AgentsModule, AssistantModule.forRoot(assistantConfig)], | ||
imports: [AgentsModule, AssistantModule.register(assistantConfig)], | ||
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. Could you update the
|
||
providers: [ChatSockets], | ||
}) | ||
export class ChatModule {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,26 @@ | ||
import { Module } from '@nestjs/common'; | ||
import { DynamicModule, Module } from '@nestjs/common'; | ||
import { AiService } from './ai.service'; | ||
import { AiController } from './ai.controller'; | ||
import { createControllerWithPrefix } from '../../utils/controllers'; | ||
|
||
@Module({ | ||
providers: [AiService], | ||
controllers: [AiController], | ||
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 that case we have multiple controllers, please remove it. |
||
exports: [AiService], | ||
}) | ||
export class AiModule {} | ||
export class AiModule { | ||
static register(prefix: string): DynamicModule { | ||
return { | ||
module: AiModule, | ||
providers: [ | ||
AiService, | ||
{ | ||
provide: 'PREFIX', | ||
useValue: prefix, | ||
}, | ||
], | ||
controllers: [createControllerWithPrefix(AiController, prefix)], | ||
exports: [AiService], | ||
}; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,28 @@ | ||
import { Module } from '@nestjs/common'; | ||
import { DynamicModule } from '@nestjs/common'; | ||
import { AiModule } from '../ai'; | ||
import { RunModule } from '../run'; | ||
import { ChatHelpers } from './chat.helpers'; | ||
import { ChatService } from './chat.service'; | ||
import { SocketModule } from '@nestjs/websockets/socket-module'; | ||
import { ChatController } from './chat.controller'; | ||
import { createControllerWithPrefix } from '../../utils/controllers'; | ||
|
||
export const sharedServices = [ChatHelpers, ChatService]; | ||
|
||
@Module({ | ||
imports: [SocketModule, AiModule, RunModule], | ||
providers: [...sharedServices], | ||
controllers: [ChatController], | ||
exports: [...sharedServices], | ||
}) | ||
export class ChatModule {} | ||
export class ChatModule { | ||
static register(prefix: string): DynamicModule { | ||
return { | ||
module: ChatModule, | ||
imports: [SocketModule, AiModule, RunModule], | ||
providers: [ | ||
...sharedServices, | ||
{ | ||
provide: 'PREFIX', | ||
useValue: prefix, | ||
}, | ||
], | ||
controllers: [createControllerWithPrefix(ChatController, prefix)], | ||
exports: [...sharedServices], | ||
}; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,28 @@ | ||
import { Module } from '@nestjs/common'; | ||
import { DynamicModule, Module } from '@nestjs/common'; | ||
import { FilesService } from './files.service'; | ||
import { FilesController } from './files.controller'; | ||
import { AiModule } from '../ai'; | ||
import { createControllerWithPrefix } from '../../utils/controllers'; | ||
|
||
@Module({ | ||
imports: [AiModule], | ||
providers: [FilesService], | ||
controllers: [FilesController], | ||
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 that case we have multiple controllers, please remove it. |
||
exports: [FilesService], | ||
}) | ||
export class FilesModule {} | ||
export class FilesModule { | ||
static register(prefix: string): DynamicModule { | ||
return { | ||
module: FilesModule, | ||
providers: [ | ||
AiModule, | ||
{ | ||
provide: 'PREFIX', | ||
useValue: prefix, | ||
}, | ||
], | ||
controllers: [createControllerWithPrefix(FilesController, prefix)], | ||
exports: [FilesService], | ||
}; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,28 @@ | ||
import { Module } from '@nestjs/common'; | ||
import { DynamicModule, Module } from '@nestjs/common'; | ||
import { ThreadsController } from './threads.controller'; | ||
import { ThreadsService } from './threads.service'; | ||
import { AiModule } from '../ai'; | ||
import { createControllerWithPrefix } from '../../utils/controllers'; | ||
|
||
@Module({ | ||
imports: [AiModule], | ||
providers: [ThreadsService], | ||
controllers: [ThreadsController], | ||
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 that case we have multiple controllers, please remove it. |
||
exports: [ThreadsService], | ||
}) | ||
export class ThreadsModule {} | ||
export class ThreadsModule { | ||
static register(prefix: string): DynamicModule { | ||
return { | ||
module: ThreadsModule, | ||
providers: [ | ||
ThreadsService, | ||
{ | ||
provide: 'PREFIX', | ||
useValue: prefix, | ||
}, | ||
], | ||
controllers: [createControllerWithPrefix(ThreadsController, prefix)], | ||
exports: [ThreadsService], | ||
}; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
import { Controller, Get } from '@nestjs/common'; | ||
import { createControllerWithPrefix } from './controllers'; | ||
import 'reflect-metadata'; | ||
|
||
@Controller('test') | ||
class TestController { | ||
@Get() | ||
getTest(): string { | ||
return 'Original Controller'; | ||
} | ||
} | ||
|
||
describe('createControllerWithPrefix', () => { | ||
it('should create a new controller with the prefixed route', () => { | ||
const PrefixedController = createControllerWithPrefix( | ||
TestController, | ||
'api', | ||
); | ||
|
||
const originalPath = Reflect.getMetadata('path', TestController); | ||
const prefixedPath = Reflect.getMetadata('path', PrefixedController); | ||
|
||
expect(originalPath).toBe('test'); | ||
|
||
expect(prefixedPath).toBe('api/test'); | ||
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. I can't image the case that we would like to keep both version - with and without prefix. IMO user should be able to decide and select only one option by providing prefix or not. |
||
}); | ||
|
||
it('should throw an error if the controller does not have @Controller decorator', () => { | ||
class InvalidController {} | ||
|
||
expect(() => createControllerWithPrefix(InvalidController, 'api')).toThrow( | ||
'The provided controller does not have a @Controller decorator.', | ||
); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
import { Controller } from '@nestjs/common'; | ||
import { Type } from '@nestjs/common/interfaces'; | ||
import 'reflect-metadata'; | ||
|
||
export function createControllerWithPrefix<T extends object>( | ||
controller: Type<T>, | ||
prefix: string, | ||
): Type<T> { | ||
const originalPath = Reflect.getMetadata('path', controller) as | ||
| string | ||
| undefined; | ||
|
||
if (!originalPath) { | ||
throw new Error( | ||
'The provided controller does not have a @Controller decorator.', | ||
); | ||
} | ||
|
||
const DynamicController = class extends controller {}; | ||
|
||
Controller(`${prefix}/${originalPath}`)(DynamicController); | ||
|
||
return DynamicController; | ||
} |
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.
Can you put default
assistant-prefix
as an optional value - empty string?