-
Notifications
You must be signed in to change notification settings - Fork 368
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
feat: Convert database factory in singleton #98
feat: Convert database factory in singleton #98
Conversation
@brunofamiliar is attempting to deploy a commit to the TabNews Team on Vercel. To accomplish this, @brunofamiliar needs to request access to the Team. Afterwards, an owner of the Team is required to accept their membership request. |
infra/database.js
Outdated
@@ -1,6 +1,6 @@ | |||
import { Pool } from "pg"; | |||
|
|||
export default function DatabaseFactory() { | |||
const DatabaseFactory = () => { | |||
const poolConfiguration = { |
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.
Interessante o jeito que você implementou! Uma sugestão seria usar o próprio arquivo para ser a instância, e daí você pode tirar um nível de abstração que é criar a instância dentro da instância do arquivo, por exemplo:
// infra/database.js
import { Pool } from "pg";
const poolConfiguration = {
user: process.env.POSTGRES_USER,
host: process.env.POSTGRES_HOST,
database: process.env.POSTGRES_DB,
password: process.env.POSTGRES_PASSWORD,
port: process.env.POSTGRES_PORT,
ssl: {
rejectUnauthorized: false,
},
};
// https://github.com/filipedeschamps/tabnews.com.br/issues/84
if (["test", "development"].includes(process.env.NODE_ENV) || process.env.CI) {
delete poolConfiguration.ssl;
}
const pool = new Pool(poolConfiguration);
async function query(query) {
const results = await pool.query(query);
return results;
}
async function getNewConnectedClient() {
// When manually creating a new connection like this,
// you need to make sure to close it afterward
// with the .end() method.
return await pool.connect();
}
export default {
query,
getNewConnectedClient,
};
PS: eu já estava com esse arquivo pronto, porque na RFC das milestones estava anotado para eu avaliar a possibilidade de implementar isso, mas daí para eu conseguir avaliar tive que implementar 😂
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.
Acabei de realizar a modificação 😄. Perdão pela demora, estava meio corrido essa semana. Ainda tem uma coisa me incomodando, quem tiver alguma sugestão, me avise por favor. Ainda da forma que está, o usuário precisa sempre finalizar com o client.end()
. Seria interessante uma forma de abstrair, para que a classe database realize automaticamente essa operação. Tentei pensar em uma forma, porém não surgiu nada em mente 😕😕
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.
Showww @brunofamiliar ! Então, normalmente iremos utilizar o database.query()
para fazer as consultas, e o Pool
irá se responsabilizar por gerenciar as conexões 👍
Apenas nesse caso dos testes ou em transactions que iremos ter que gerenciar o client (pois a transaction fica presa ao client). No caso da transaction, poderemos talvez criar uma abstração, do tipo:
const transaction = await database.transaction();
await transaction.query(...);
await transaction.query(...);
await transaction.commit(); // ou .rollback()
// esse objeto transaction é um novo client, no final das contas
Não sei, só rabisquei e no final das contas seria só um "method sugar" pra uma transaction presa a um client.
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.
Show!! Vou começar pensar na implementação dessa abstração aqui ;)
tests/orchestrator.js
Outdated
@@ -36,7 +35,6 @@ export default function orchestratorFactory() { | |||
} | |||
|
|||
async function dropAllTables() { | |||
const database = databaseFactory(); | |||
await database.query("drop schema public cascade; create schema public;"); | |||
await database.pool.end(); |
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.
Eu acho que isso não irá funcionar, ao menos testei sua branch aqui local e não rodou e o motivo disso é que o Pool agora está compartilhado e é um singleton, correto? Com isso, você cria o Pool, faz a query que dropa as tabelas, e em seguida fecha o Pool, e se ele foi manualmente fechado, não existe mais e não irá gerenciar novas conexões.
Então você não pode fechar o Pool, mas também não pode só rodar a query, porque se só rodar a query, os testes vão passar, mas o jest
vai reclamar que ficou aberto um handle
, que é justamente o Pool segurando a conexão ao banco. Então dentro dos testes você não deve mexer com o Pool e deve usar manualmente uma instância do banco, porque daí sim você fecha ela em seguida, por exemplo:
async function dropAllTables() {
const databaseClient = await database.getNewConnectedClient();
await databaseClient.query(
"drop schema public cascade; create schema public;"
);
await databaseClient.end();
}
Por isso nos testes E2E foi importante subir o serviço do Next (que também conecta ao banco usando o Pool) por fora do processo do jest
, na parte da infraestrutura. Porque daí o jest
fica livre de Pools, e a camada de infraestrutura sobe e derruba o site nos momentos certos.
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.
Eu tinha esquecido de rodar os tests, mals ai kk
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.
Adicionei dois comentários em 2 arquivos que acredito valer a pena considerar 🤝 👍
Oopa, obrigado pelo comentário @filipedeschamps. Já vou providenciar aqui os ajustes |
Show! Qualquer blocker que encontrar, me avisa 🤝 👍 Em paralelo, já imaginou se esse projeto depois de uns anos vira algo gigante, e foi você quem transformou o database em singleton e todo mundo ta usando? 😍 Eu acho muito massa pensar essas coisas 😂 👍 |
Nossa.. genial!! 😍 Seria uma honra pode dizer que contruibo com um projeto grandioso desses, ao lado de todas essas feras 😄💥 |
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.
Rodei aqui, e tudo certinho 😍 show de bola!!!! Vou fazer o merge apenas na abertura da Milestone, que tudo indica que será na sexta-feira 👍
Realmente se tornou um projeto gigante! |
Proposta para refatoração padrão factory da instancia de 'database'
Pessoal, implementei inicialmente uma proposta para conversão do padrão factory para singleton do arquivo de database como foi sugerido no PR #92
O que foi feito?