the modules with controllers were modified to be dynamic#85
the modules with controllers were modified to be dynamic#85arodriguezcb wants to merge 1 commit intoboldare:mainfrom
Conversation
arodriguezcb
force-pushed
the
feature/dynamic-modules
branch
from
2339996 to
f1391ff
Compare
sebastianmusial
left a comment
sebastianmusial
left a comment
There was a problem hiding this comment.
Thank you so much, @arodriguezcb, for your contributions to the project! 🎉 I'm really excited about the new features. Please take a look at my comments and let me know if you have some questions. 🙌
There was a problem hiding this comment.
Due to changes in this file when I run npm run start:dev command I receive error:
"The Nx Daemon is unsupported in WebAssembly environments. Some things may be slower than or not function as expected."
|
|
||
| @Module({ | ||
| imports: [AgentsModule, AssistantModule.forRoot(assistantConfig)], | ||
| imports: [AgentsModule, AssistantModule.register(assistantConfig)], |
There was a problem hiding this comment.
Could you update the readme file:
forRoot->register- new property in the configuration
| export const assistantConfig: AssistantConfigParams = { | ||
| id: process.env['ASSISTANT_ID'] || '', | ||
| params: assistantParams, | ||
| assistantPrefix: 'assistant-prefix', |
There was a problem hiding this comment.
Can you put default assistant-prefix as an optional value - empty string?
| export const assistantConfig: AssistantConfigParams = { | ||
| id: process.env['ASSISTANT_ID'] || '', | ||
| params: assistantParams, | ||
| assistantPrefix: 'assistant-prefix', |
There was a problem hiding this comment.
Please check my screenshot, It looks like we have multiple instances instead of one with the prefix 🤔
In my opinion we should have only once instance and decide if we would like have prefix or not.
|
|
||
| @Module({ | ||
| providers: [AiService], | ||
| controllers: [AiController], |
There was a problem hiding this comment.
In that case we have multiple controllers, please remove it.
| @Module({ | ||
| imports: [AiModule], | ||
| providers: [FilesService], | ||
| controllers: [FilesController], |
There was a problem hiding this comment.
In that case we have multiple controllers, please remove it.
| @Module({ | ||
| imports: [AiModule], | ||
| providers: [ThreadsService], | ||
| controllers: [ThreadsController], |
There was a problem hiding this comment.
In that case we have multiple controllers, please remove it.
|
|
||
| expect(originalPath).toBe('test'); | ||
|
|
||
| expect(prefixedPath).toBe('api/test'); |
There was a problem hiding this comment.
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.
3 participants
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently, modules are loaded as singletons and with static paths, which makes it difficult to use multiple assistants configurations.
Issue Number: N/A
What is the new behavior?
Modules are created dynamically and a prefix is added to modify the paths of each loaded assitant.
Other information