Skip to content
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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

LorhanSohaky
Copy link
Member

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

  • Não sabia como poderia testar se a requisição voltou com keep-alive na resposta sem afetar a interface do cep-promise
  • Um dos testes falhou, mas creio que não está relacionado com as mudanças que fiz
  • Fiz algumas correções de code-style
  • Se forçar code-style for algo muito importante, é preciso modificar o lint-fix para algo assim:
     "lint-fix": "standard '*.js' 'src/**/*.js' 'test/**/*.js' --fix",

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling e2bb712 on LorhanSohaky:feature/keep-alive into 9bca1c4 on filipedeschamps:master.

@coveralls
Copy link

coveralls commented Oct 6, 2020

Coverage Status

Coverage decreased (-0.6%) to 99.419% when pulling 5f06013 on LorhanSohaky:feature/keep-alive into 9bca1c4 on filipedeschamps:master.

Copy link
Member

@lucianopf lucianopf left a 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? 🙏
Screen Shot 2020-10-07 at 07 56 50

src/cep-promise.js Show resolved Hide resolved
@LorhanSohaky
Copy link
Member Author

Vi agora na Newsletter do Filipe sobre o Hacktoberfest. Acham possível eleger esse PR? Assim poderei participar com este PR

src/cep-promise.js Show resolved Hide resolved
README.md Outdated
const agent = new https.Agent({ keepAlive: true })
const proxyURL = 'socks5://127.0.0.1:1950'

const configurations = { agent, proxyURL }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LorhanSohaky

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!

Copy link
Member Author

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: '...'
    }
  }
}

Copy link

@ivancorrea ivancorrea Oct 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LorhanSohaky

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'
		}
	}
}

Copy link
Member Author

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

Copy link
Member Author

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?

@LorhanSohaky LorhanSohaky changed the title feature/keep-alive Passar diversas configurações por parâmetro e adição de keep-alive Nov 13, 2020
@LorhanSohaky LorhanSohaky changed the title Passar diversas configurações por parâmetro e adição de keep-alive Configurações da biblioteca e adição de keep-alive Nov 13, 2020
@lucianopf
Copy link
Member

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 😬

#197

@LorhanSohaky
Copy link
Member Author

@lucianopf , fera demais

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants