Conversation
Signed-off-by: Erick Rodrigues <erick.rodrigues.santana14@gmail.com>
Signed-off-by: Erick Rodrigues <erick.rodrigues.santana14@gmail.com>
Signed-off-by: Erick Rodrigues <erick.rodrigues.santana14@gmail.com>
add mongo uri env var to make our repo passable as argument of our frontend Signed-off-by: Erick Rodrigues <erick.rodrigues.santana14@gmail.com>
Signed-off-by: Erick Rodrigues <erick.rodrigues.santana14@gmail.com>
Signed-off-by: Erick Rodrigues <erick.rodrigues.santana14@gmail.com>
jooaodanieel
left a comment
jooaodanieel
left a comment
There was a problem hiding this comment.
FIquei tranquilo com a alteração. Uma parada que me chamou a atenção, fiquei apenas me perguntando: será que vale apagar as interfaces que antes o Mongoose usava?
As que estão em collector/src/core/interfaces
| MONGO_DB= | ||
| MONGO_URI= |
There was a problem hiding this comment.
Acho que é mais legal usar a URL completa informando host, port e db -- lembrando que assim a gente pode usar o nosso próprio repo como exemplo de input.
There was a problem hiding this comment.
Antes a gente não tava usando, nesse PR eu mudei para usar a url completa justamente por esse motivo. A URL eu pego completa no meu .env que eu to usando: MONGO_URI=mongodb://root:collector123@collector_db:27017/collector?authSource=admin. Esse MONGO_DB é só uma variável que contém o nome do BD, para eu conseguir acessar no código fazendo mongoClient.db(process.env.MONGO_DB). Com isso, eu teria acesso as collections de dentro do banco de dados da aplicação. Se eu não colocase essa env var, eu teria que parsear a URL pra encontrar o nome do BD (não queria ter esse trampo).
There was a problem hiding this comment.
Vê na documentação do mongo se quando você conecta usando a URL completa e chama conn.db() vazio, ele já não pega o que veio na URL
| class SystemModel { | ||
| constructor(private readonly sys: any) {} | ||
|
|
||
| save(): Promise<any> { | ||
| console.log('system saved'); | ||
| return new Promise((resolve) => resolve(this.sys)); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Isso é algum vestígio esquecido? Se não for, tem cara de que tá no lugar errado.
There was a problem hiding this comment.
Essa classe eu criei meio suja mesmo porque lá na `RegisterSystem.run()' a gente cria uma instancia desse SystemModel e chama um método save dela. Tecnicamente o Bruno vai mudar isso na parte dele, então eu preferi criar essa classe meio que mockada mesmo só pra não modificar nada do que ele vai mexer lá, mas confesso que fiquei um pouco na dúvida se precisava ou não disso. Qual a melhor solução nesse caso?
There was a problem hiding this comment.
Não sei se eu entendi direito a origem dessa classe...
Quando tivermos call, a gente fala sobre isso, pode ser? Fica na pilha
| const mongoDb = process.env.MONGO_DB || 'test'; | ||
| const mongoUri = process.env.MONGO_URI || 'mongodb://localhost:27017'; |
There was a problem hiding this comment.
Idem sobre URL completa no .env
There was a problem hiding this comment.
Respondi lá sobre o que eu pensei nesse caso.
| export class DatabaseConnection { | ||
| private static client: MongoClient; | ||
|
|
||
| export function setupDatabaseConnection(): void { | ||
| mongoose | ||
| .connect(`${mongoUrl}/${mongoDb}?authSource=admin`, { | ||
| useNewUrlParser: true, | ||
| useUnifiedTopology: true, | ||
| }) | ||
| .then((_) => { | ||
| console.log('Database connected '); | ||
| }) | ||
| .catch((error) => { | ||
| console.log('Error: ', error); | ||
| }); | ||
| public static async connect(): Promise<void> { | ||
| if (!this.client) { | ||
| try { | ||
| this.client = new MongoClient(mongoUri, { | ||
| useNewUrlParser: true, | ||
| useUnifiedTopology: true, | ||
| }); | ||
|
|
||
| await this.client.connect(); | ||
| console.log('- database connected'); | ||
| } catch (error) { | ||
| console.log('- error while connecting to database: ', error); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public static async getDb(): Promise<Db> { | ||
| if (!this.client) { | ||
| await this.connect(); | ||
| } | ||
|
|
||
| return this.client.db(mongoDb); | ||
| } |
There was a problem hiding this comment.
Qual o motivo de fazer as coisas como static?
There was a problem hiding this comment.
Aliás isso tem uma carinha de sair um Singleton, conhece?
There was a problem hiding this comment.
Foi exatamente nesse pattern que eu me baseei. A ideia foi criar uma instância única de conexão com o banco de dados para toda a aplicação.
Antes de adotá-la, eu fiquei pensando em como eu passaria o BD para as classes que precisam utilizá-lo. Se a gente estivesse usando o Koa, eu conseguiria injetar o bd em um objeto context global, e aí seria mais fácil o controller ter acesso: era só obter desse objeto context, da mesma forma que eu faço com a request e a response no Koa. Porém o express não dá suporte a isso pelo que eu vi. E também não queria mudar a estrutura do RegisterController nem nada do tipo.
De toda forma, o que eu queria fazer era simples: criar uma instância de conexão com o bd quando minha aplicação iniciasse, e passar essa mesma instância para quem precisa usar. Por isso eu adotei uma parada meio Singleton, por isso o uso dos static. Porém, se fosse tiver uma sugestão melhor, sou todo ouvidos.
There was a problem hiding this comment.
Não rola fazer via injeção de dependências? Talvez tenha que quebrar com o lance de static
@jooaodanieel sobre isso, eu acho melhor não apagar, porque eu gosto de tipar as coisas e eu to usando essas interfaces no meu PR dos visitors lá |
Mas nesse caso as interfaces estão redundantes, porque já tem classes idênticas. |
3 participants
Add mongodb driver instead of mongoose