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

Production declaration files #74

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

Conversation

davidpvilaca
Copy link
Contributor

This close #73

Proposed Changes

  • Refactor to TypeScript lang
  • Add types
  • In production generate declaration files (.d.ts)

Motivation and Context

As I said in issue #73 I am using a scribe and I already had to write some types at the root of the my project to get rid of using "any" in everything related to this library.

How Has This Been Tested?

  • NodeJS v12.18.4, NPM v6.14.9
  • npm run lint
  • npm test
  • npm run build (dist folder checked with .d.ts files)
  • install local in example project and used like a http example

Impactful Changes

  • add babel transpiler and tsc to generate declaration files
  • use Jest to tests instead of ava
  • update dependencies

Any change or improvement to approve this PR, I am ready to listen

@coveralls
Copy link

Coverage Status

Coverage increased (+5.4%) to 80.392% when pulling 0d77da5 on davidpvilaca:refactor/typescript into c777738 on pagarme:master.

@claytonsilva
Copy link

@davidpvilaca Antes de tudo gostaria muito de agradecer pela sua contribuição!!!

Tenho alguns pontos de ressalva para ponderar nela:

  • Temos muito contexto dentro de uma PR. inicialmente achei que se tratava de adicionar declaration files, conforme diz o título dela, mas vejo que ela é um refactor em typescript.
  • Dentro desse refactor, vemos que não somente troca para typescript, como também troca a suíte de testes e realiza bump de versões que estão defasadas.

Proponho quebrar essa pr nos seguintes pontos:

  • Bump das versões dos pacotes
  • Mudança do suíte de testes, mas acho que não precisa mudar ainda para o sufixo *.spec.* acho que pode deixar para outro momento essa mudança, os arquivos ainda estão separados dentro de /unit/
  • Por fim a mudança pra typescript

Por fim tem que mudar esse descritivo da PR. 😄

Se seguir nessa ordem as mudanças acho que fica bem legal.

const stringify = log => {
if (R.is(String, log)) return log
export const stringify = (log: any) => {
if (typeof log === 'string') return log
Copy link
Contributor

Choose a reason for hiding this comment

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

Fala @davidpvilaca, talvez isso aqui dê problema. 🤔
Manter o comportamento com Ramda tanto aqui, quanto nos outros locais modificados dessa forma é mais seguro

@@ -0,0 +1,25 @@
const { compilerOptions } = require('./tsconfig.json')
Copy link
Contributor

Choose a reason for hiding this comment

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

Obrigado por colocar Jest 😍

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

Successfully merging this pull request may close these issues.

Declaration files .d.ts
4 participants