-
Notifications
You must be signed in to change notification settings - Fork 316
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
Configurações da biblioteca e adição de keep-alive #191
base: master
Are you sure you want to change the base?
Conversation
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.
Opa manão que PR bom de revisar! Parabéns e MUITO obrigado! 🚀
Revisando o código parece tudo 100% e ainda melhorou algumas coisinhas 🔝 !!
Vou segurar o approve um pouquinho só por 1 motivo (prometo que é rápido hehehe):
- Quero pensar se a gnt consegue fazer uns testes nessa implementação (se não conseguir a gnt toca o barco assim msm)
@filipedeschamps corre aqui pra ver esse PR que massa! 😱 🤘
Se já tiver por aí aproveita e da um helpzinho nos hooks do travis? 🙏
Vi agora na Newsletter do Filipe sobre o Hacktoberfest. Acham possível eleger esse PR? Assim poderei participar com este PR |
README.md
Outdated
const agent = new https.Agent({ keepAlive: true }) | ||
const proxyURL = 'socks5://127.0.0.1:1950' | ||
|
||
const configurations = { agent, proxyURL } |
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.
Aproveitaria este PR e adicionaria novas configurações, como o timeout da requisição, que já foi adicionada pelo @filipedeschamps no PR #190 e também headers, como o User-Agent (para quando rodar do lado do servidor) que eu ajustei no PR #189.
Não esquecer do valor default destas novas configurações.
Dessa forma podemos cancelar os PRs anteriores e resolver tudo neste!
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.
@ivmarcos , para fazer isso, precisamos decidir se serão passadas as mesmas configurações para todos os serviços ou cada um pode receber uma configuração diferente (vide exemplo #190). E se for um para cada provedor, qual será o formato para isso.
O que tenho em mente é fazer algo parecido com:
const config = {
providersConfig: {
brasilapi: {
timeout: 1000,
proxyURL: '...'
}
}
}
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.
Exatamente! Cada provider precisa receber configurações específicas, mas acredito que poderia ter uma opção pra definir geral:
const config = {
providersConfig: {
//specific providers
brasilapi: {
timeout: 1000,
proxyURL: '...',
headers: {
'user-agent': 'especific ua'
}
},
// for all providers
timeout: 2000,
headers: {
'user-agent': 'cep-promise'
}
}
}
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.
Modifiquei, agora suporta sobrescrever os headers e adicionar timeout
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.
@ivancorrea consegue fazer o review?
Mestres, só pra não deixar vcs perdidos abri uma issue com um planinho de ação pra reorganizar o repo dado a migração pra org do BrasilAPI 😬 |
@lucianopf , fera demais |
Motivação
Dependendo da quantidade de requisições feitas, perde-se muito tempo fazendo o handshaking e isso pode ser facilmente resolvido com o cabeçalho keep-alive. A implementação foi baseada na issue#423 do node-fetch.
Implementação
Quando estava implementando fiquei na dúvida se era uma boa escolha colocar isso como default ou uma configuração da lib, então para evitar que essa modificação cause algum problema para os demais desenvolvedores preferi passar como uma configuração. E com isso aproveitei para possibilitar que as proxies também sejam passadas como configuração aos serviços.
Por fim, agora caso queiramos passar qualquer tipo de configuração aos serviços não precisaremos mais (talvez / espero eu hahaha) modificar a interface.
Observações
lint-fix
para algo assim: