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

style/imports sort #1496

Closed

Conversation

imattferreira
Copy link

@imattferreira imattferreira commented Aug 9, 2023

Salve galera!!

Esse PR faz a reimplementação do #552 e que foi levandado novamente pelo @aprendendofelipe em #1494.

Nesse PR foi adicionado um novo plugin do prettier, o @trivago/prettier-plugin-sort-imports, e, em 5dfb0df aproveitei e transformei alguns imports que estavam no padrão commonJS para ESModules.

@vercel
Copy link

vercel bot commented Aug 9, 2023

@MattFerreira18 is attempting to deploy a commit to the TabNews Team on Vercel.

A member of the Team first needs to authorize it.

@imattferreira imattferreira marked this pull request as ready for review August 9, 2023 02:26
Copy link
Collaborator

@aprendendofelipe aprendendofelipe left a comment

Choose a reason for hiding this comment

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

Fala @MattFerreira18, muito obrigado pelo PR! 💪💪💪

Analisando seu PR anterior (#552), gostei mais da sua primeira proposta que era usando só o ESLint. Só que faltavam alguns ajustes nela, além de fazer funcionar corretamente o script lint:fix, já que faltava o "ponto" (.) no package.json. Só pela falta desse ponto é que não funcionou a correção automática na sua primeira sugestão, mas é um problema que já estava presente antes da sua proposta, e ainda está presente no código 😅

Fora isso, tem duas coisas presentes nas duas versões (com eslint ou prettier) que me incomodam um pouco:

  1. A configuração necessária para separar as importações internas e externar. Especificamente o fato de precisar listar as principais pastas do projeto no arquivo de configuração, pois isso não é intuitivo para qualquer um contribuidor que adicionar novas pastas no projeto futuramente. Inclusive no PR faltou adicionar o alias @/, pois senão o import from '@/TabNewsUI' fica agrupado com as importações externas.

  2. A ordem de classificação em boa parte dos arquivos não está compatível com a organização automática do VS Code, ou seja, dependendo da configuração do editor, por exemplo se estiver organizando automaticamente ao salvar, muitos arquivos vão dar trabalho desnecessário para os contribuidores.

Vendo como ajustar essas coisas, percebi que não precisamos adicionar nenhuma nova biblioteca, mas apenas configurar adequadamente o sort-imports do ESLint e o plugin eslint-plugin-import que já está presente por causa do eslint-config-next e também pelo eslint-plugin-primer-react.

Como seriam muitas mudanças para sugerir nesse PR, preferi abrir o #1499, mas como você também já dedicou tempo para resolver esses problemas, seria muito bom ter sua revisão por lá, pelo menos no primeiro commit (01cbb3a), onde o arquivo .eslintrc é a única modificação relevante fora a correção do script no package.json. O restante são só as adequações dos demais arquivos para as novas regras. 💪💪💪

O problema 1 foi resolvido configurando o path no resolver e adicionando @/** em pathGroups:

"settings": {
  "import/resolver": {
    "node": {
      "paths": ["."]
    }
  }
}

e

"pathGroups": [
  {
   "pattern": "@/**",
   "group": "internal"
  }
]

Já o problema 2 foi resolvido para quase todos os arquivos com o restante das configurações. A exceção são dois arquivos (pages/[username] e TabCoinsButtons) que possuem essas duas linhas:

import { ... } from 'next/router';
import { ... } from 'next-swr';

Onde o ESLint deixa na ordem acima, pois considera next antes de next-swr, mas o VS Code leva em consideração next/router com a barra sendo um caractere e não um divisor, o que inverte a ordem de classificação.

Nos últimos commits eu aproveitei para ajustar outras configurações do ESLint, mas não tem relação com os imports.

@imattferreira
Copy link
Author

Fala @MattFerreira18, muito obrigado pelo PR! 💪💪💪

Analisando seu PR anterior (#552), gostei mais da sua primeira proposta que era usando só o ESLint. Só que faltavam alguns ajustes nela, além de fazer funcionar corretamente o script lint:fix, já que faltava o "ponto" (.) no package.json. Só pela falta desse ponto é que não funcionou a correção automática na sua primeira sugestão, mas é um problema que já estava presente antes da sua proposta, e ainda está presente no código 😅

Fora isso, tem duas coisas presentes nas duas versões (com eslint ou prettier) que me incomodam um pouco:

1. A configuração necessária para separar as importações internas e externar. Especificamente o fato de precisar listar as principais pastas do projeto no arquivo de configuração, pois isso não é intuitivo para qualquer um contribuidor que adicionar novas pastas no projeto futuramente. Inclusive no PR faltou adicionar o alias `@/`, pois senão o `import from '@/TabNewsUI'` fica agrupado com as importações externas.

2. A ordem de classificação em boa parte dos arquivos não está compatível com a organização automática do VS Code, ou seja, dependendo da configuração do editor, por exemplo se estiver organizando automaticamente ao salvar, muitos arquivos vão dar trabalho desnecessário para os contribuidores.

Vendo como ajustar essas coisas, percebi que não precisamos adicionar nenhuma nova biblioteca, mas apenas configurar adequadamente o sort-imports do ESLint e o plugin eslint-plugin-import que já está presente por causa do eslint-config-next e também pelo eslint-plugin-primer-react.

Como seriam muitas mudanças para sugerir nesse PR, preferi abrir o #1499, mas como você também já dedicou tempo para resolver esses problemas, seria muito bom ter sua revisão por lá, pelo menos no primeiro commit (01cbb3a), onde o arquivo .eslintrc é a única modificação relevante fora a correção do script no package.json. O restante são só as adequações dos demais arquivos para as novas regras. 💪💪💪

O problema 1 foi resolvido configurando o path no resolver e adicionando @/** em pathGroups:

"settings": {
  "import/resolver": {
    "node": {
      "paths": ["."]
    }
  }
}

e

"pathGroups": [
  {
   "pattern": "@/**",
   "group": "internal"
  }
]

Já o problema 2 foi resolvido para quase todos os arquivos com o restante das configurações. A exceção são dois arquivos (pages/[username] e TabCoinsButtons) que possuem essas duas linhas:

import { ... } from 'next/router';
import { ... } from 'next-swr';

Onde o ESLint deixa na ordem acima, pois considera next antes de next-swr, mas o VS Code leva em consideração next/router com a barra sendo um caractere e não um divisor, o que inverte a ordem de classificação.

Nos últimos commits eu aproveitei para ajustar outras configurações do ESLint, mas não tem relação com os imports.

(mals pela demora, fiquei enrolado com algumas coisas da facul e do job) showw, vou dar uma olhada no seu PR sim. Vou aproveitar e fechar esse daqui tbm

@aprendendofelipe
Copy link
Collaborator

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.

None yet

2 participants