search

channel-io / bezier-react

React components library that implements Bezier design system.
https://main--62bead1508281287d3c94d25.chromatic.com
Apache License 2.0
213 stars 46 forks source link

Enhance code formatting rules #1211

Closed sungik-choi closed 1 year ago

sungik-choi commented 1 year ago

Self Checklist

Related Issue

Fixes #1210

Summary

ESLint 규칙을 개선하고, 코드 전반을 포매팅합니다.

Details

New Rule

아래 규칙을 적용하고자 했습니다.

  1. 일관적이지 않은 import 개행 규칙을 통일합니다.
  2. import, export 시 type 키워드가 가능한 경우 일관적으로 추가하도록 통일합니다.
// import item이 하나 이상일 경우 개행합니다.
'import-newlines/enforce': ['error', { items: 1 }],
// '@typescript-eslint/consistent-type-imports' 과 세트입니다.
// import / type import가 나뉘며 중복으로 생기는 import를 제거합니다. 그 과정에서 type 키워드를 '인라인' 으로 추가합니다.
'import/no-duplicates': ['error', { 'prefer-inline': true }],
// module import 시 type일 경우 type 키워드를 '인라인'으로 추가합니다. (ex. import { type FooType } from './foo')
'@typescript-eslint/consistent-type-imports': ['error', { fixStyle: 'inline-type-imports' }],
// module export 시 type일 경우 type 키워드를 추가합니다.
'@typescript-eslint/consistent-type-exports': 'error',

또한 @channel.io/eslint-config 에서 불필요하게 비활성화하고 있던 a11y 관련 규칙을 warn level로 활성화합니다.

  1. 당장 수정이 어려운 부분이 있고
  2. warning이 발생하는 부분이 많지 않아

우선은 warn level로 유지하는 방향을 선택했습니다. 추후 error level로 변경하고 개선해야합니다.

'jsx-a11y/alt-text': 'warn',
'jsx-a11y/click-events-have-key-events': 'warn',
'jsx-a11y/no-static-element-interactions': 'warn',

About Prettier

Prettier는 다음과 같은 이유로 적용하지 않았습니다.

  1. 현재 데스크의 ESLint 설정(@channel.io/eslint-config)은 코드 포매터의 역할도 겸하고 있습니다. hook deps의 개행 규칙처럼 Prettier가 하는 역할보다 훨씬 다양한 역할을 수행하고 있어서, eslint-config-prettier 를 적용해서 Prettier와 충돌하는 ESLint 규칙을 비활성화하더라도, 기존 ESLint가 하던 역할을 Prettier가 온전히 대체할 수 없습니다. bezier-react는 오픈소스이지만, 채널 웹팀이 주로 관리하고 있는 프로젝트이기에 채널 웹팀의 코드 컨벤션에 맞추는 편이 개발자 경험에 훨씬 나은 방향이라고 판단했습니다.
  2. 당장 적용할만한 규칙이 없습니다. printWidth, arrowParens 같은 규칙은 이미 팀의 컨벤션으로 따로 정해져 있지 않은 상태(rule off)이기에 굳이 강제할 필요가 없다고 판단했습니다. (정하더라도 ESLint의 규칙으로 설정하면 충분하다고 판단했습니다)

이미 pre-commit 훅에 lint 과정이 포함되어있고(lint-staged), 에디터에 format on auto-save 기능이 있으므로 pre-commit에 auto fix 커맨드를 추가하진 않았습니다.

etc

Breaking change or not (Yes/No)

No

References

changeset-bot[bot] commented 1 year ago

⚠️ No Changeset found

Latest commit: 00661bef6eb6c26d386ffb197134a3976cf50fb6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions[bot] commented 1 year ago

Chromatic Report

🚀 Congratulations! Your build was successful!

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 95.72% and project coverage change: +0.07 :tada:

Comparison is base (2476407) 77.93% compared to head (00661be) 78.00%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## next-v1 #1211 +/- ## =========================================== + Coverage 77.93% 78.00% +0.07% =========================================== Files 298 292 -6 Lines 3802 3788 -14 Branches 843 843 =========================================== - Hits 2963 2955 -8 + Misses 554 548 -6 Partials 285 285 ``` | [Impacted Files](https://codecov.io/gh/channel-io/bezier-react/pull/1211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=channel-io) | Coverage Δ | | |---|---|---| | [...eact/src/components/Avatars/Avatar/Avatar.types.ts](https://codecov.io/gh/channel-io/bezier-react/pull/1211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=channel-io#diff-cGFja2FnZXMvYmV6aWVyLXJlYWN0L3NyYy9jb21wb25lbnRzL0F2YXRhcnMvQXZhdGFyL0F2YXRhci50eXBlcy50cw==) | `100.00% <ø> (ø)` | | | [...omponents/Avatars/AvatarGroup/AvatarGroup.types.ts](https://codecov.io/gh/channel-io/bezier-react/pull/1211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=channel-io#diff-cGFja2FnZXMvYmV6aWVyLXJlYWN0L3NyYy9jb21wb25lbnRzL0F2YXRhcnMvQXZhdGFyR3JvdXAvQXZhdGFyR3JvdXAudHlwZXMudHM=) | `100.00% <ø> (ø)` | | | [.../Avatars/CheckableAvatar/CheckableAvatar.styled.ts](https://codecov.io/gh/channel-io/bezier-react/pull/1211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=channel-io#diff-cGFja2FnZXMvYmV6aWVyLXJlYWN0L3NyYy9jb21wb25lbnRzL0F2YXRhcnMvQ2hlY2thYmxlQXZhdGFyL0NoZWNrYWJsZUF2YXRhci5zdHlsZWQudHM=) | `0.00% <0.00%> (ø)` | | | [...onents/Avatars/CheckableAvatar/CheckableAvatar.tsx](https://codecov.io/gh/channel-io/bezier-react/pull/1211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=channel-io#diff-cGFja2FnZXMvYmV6aWVyLXJlYWN0L3NyYy9jb21wb25lbnRzL0F2YXRhcnMvQ2hlY2thYmxlQXZhdGFyL0NoZWNrYWJsZUF2YXRhci50c3g=) | `0.00% <0.00%> (ø)` | | | [...bezier-react/src/components/Banner/Banner.types.ts](https://codecov.io/gh/channel-io/bezier-react/pull/1211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=channel-io#diff-cGFja2FnZXMvYmV6aWVyLXJlYWN0L3NyYy9jb21wb25lbnRzL0Jhbm5lci9CYW5uZXIudHlwZXMudHM=) | `100.00% <ø> (ø)` | | | [...bezier-react/src/components/Button/Button.types.ts](https://codecov.io/gh/channel-io/bezier-react/pull/1211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=channel-io#diff-cGFja2FnZXMvYmV6aWVyLXJlYWN0L3NyYy9jb21wb25lbnRzL0J1dHRvbi9CdXR0b24udHlwZXMudHM=) | `100.00% <ø> (ø)` | | | [...es/bezier-react/src/components/Divider/Divider.tsx](https://codecov.io/gh/channel-io/bezier-react/pull/1211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=channel-io#diff-cGFja2FnZXMvYmV6aWVyLXJlYWN0L3NyYy9jb21wb25lbnRzL0RpdmlkZXIvRGl2aWRlci50c3g=) | `100.00% <ø> (ø)` | | | [...s/bezier-react/src/components/Emoji/Emoji.types.ts](https://codecov.io/gh/channel-io/bezier-react/pull/1211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=channel-io#diff-cGFja2FnZXMvYmV6aWVyLXJlYWN0L3NyYy9jb21wb25lbnRzL0Vtb2ppL0Vtb2ppLnR5cGVzLnRz) | `100.00% <ø> (ø)` | | | [...ackages/bezier-react/src/components/Emoji/index.ts](https://codecov.io/gh/channel-io/bezier-react/pull/1211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=channel-io#diff-cGFja2FnZXMvYmV6aWVyLXJlYWN0L3NyYy9jb21wb25lbnRzL0Vtb2ppL2luZGV4LnRz) | `0.00% <0.00%> (ø)` | | | [...components/Forms/FormControl/FormControl.styled.ts](https://codecov.io/gh/channel-io/bezier-react/pull/1211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=channel-io#diff-cGFja2FnZXMvYmV6aWVyLXJlYWN0L3NyYy9jb21wb25lbnRzL0Zvcm1zL0Zvcm1Db250cm9sL0Zvcm1Db250cm9sLnN0eWxlZC50cw==) | `100.00% <ø> (ø)` | | | ... and [155 more](https://codecov.io/gh/channel-io/bezier-react/pull/1211?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=channel-io) | | ... and [6 files with indirect coverage changes](https://codecov.io/gh/channel-io/bezier-react/pull/1211/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=channel-io) Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=channel-io). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=channel-io)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

Kanary159357 commented 1 year ago

About Prettier 관련

  1. ESLint와 Prettier는 서로 양립할 수 있는 포맷터입니다. 꼭 Prettier를 쓴다고 해서 Eslint의 포맷팅을 포기해야하는 것은 아닙니다!
  2. 이 점에 대해서는 동의합니다! Prettier의 로직을 적용할만한 로직이 있을떄 추가하는 것 정도도 적당할 것 같습니다. 꼭 필요하지 않다면 불필요한 복잡성이 하나 추가되는 것과 동일하기에... 혹시 Prettier를 처음 도입하기로 생각하셨던 동기가 있으면, 그것이 ESLint만으로 해결될수 있는 문제인지에 대해서 한번 확인해보면 좋을거 같습니다!

한가지 예로 ESLint에서는 non-js 파일에 대한 포매팅이 되지 않는 것으로 알고 있는데, prettier를 통해서 이를 해결할 수 있는 케이스는 있을거 같습니다. 오늘 아침에 제가 Contributing.md를 고쳤을때의 예시가 있겠네요.

저는 Prettier를 도입하지 않더라도, printWidth를 대체하는 Lint Rule 정도는 팀원들끼리 컨벤션을 정하면 좋을거 같다고 생각합니다!

Kanary159357 commented 1 year ago

https://prettier.io/playground/#N4Igxg9gdgLgprEAuEwAUBDABAHx1tAIywDISswBKS08tAHSkaxdbZYB4ATASwDcAfM3YjOGAPRCoo9h3G9BjRpQC+IADQgIABxg9oAZ2SgMAJ1MQA7gAUzCIygwAbSxgCeRzYVMYwAazgYAGUMAFs4ABkeKDhkADNnAzgvH39AoO1faIBzZBhTAFdkkDhQwjguLgqIjChsgoxsuAAxCFNQjBg9OuQQDAKYCA0QAAsYUKcAdRGeeANMsDgg+1n+WbdesANPEGik0xhrH2yO+MTigCsDAA8gnKc4AEUCiHgzpyTNTNN93sIMcpOYbaUzRGCTHhcGAjZAADgADF8LElJj5tL0QXB9nxYpoAI4veBHHQOPoGAC0MQqFWGpjgBJ4dKOjVOSASH2KSVCPDyhU59zgAEEuqDCAM4NY4KYojF3p8QAYBc9XrE2edNDAARCoTCkAAmDU+HhOHIAYQgoVZJQMAFZhgUkgAVAEOdnyvhFACSUCqsCCYFBukFPqCMDcDzlcBUKiAA

현재 저희 Lint로는 위의 구조에서 일관적인 포맷팅이 안되는데, Prettier에서는 자동으로 맞춰주는 차이도 있겠네요!

sungik-choi commented 1 year ago

About Prettier 관련

  1. ESLint와 Prettier는 서로 양립할 수 있는 포맷터입니다. 꼭 Prettier를 쓴다고 해서 Eslint의 포맷팅을 포기해야하는 것은 아닙니다!
  2. 이 점에 대해서는 동의합니다! Prettier의 로직을 적용할만한 로직이 있을떄 추가하는 것 정도도 적당할 것 같습니다. 꼭 필요하지 않다면 불필요한 복잡성이 하나 추가되는 것과 동일하기에... 혹시 Prettier를 처음 도입하기로 생각하셨던 동기가 있으면, 그것이 ESLint만으로 해결될수 있는 문제인지에 대해서 한번 확인해보면 좋을거 같습니다!

한가지 예로 ESLint에서는 non-js 파일에 대한 포매팅이 되지 않는 것으로 알고 있는데, prettier를 통해서 이를 해결할 수 있는 케이스는 있을거 같습니다. 오늘 아침에 제가 Contributing.md를 고쳤을때의 예시가 있겠네요.

저는 Prettier를 도입하지 않더라도, printWidth를 대체하는 Lint Rule 정도는 팀원들끼리 컨벤션을 정하면 좋을거 같다고 생각합니다!

Kanary159357 commented 1 year ago

저도 prettier를 도입하려면, bezier뿐만 아니라 채널코퍼레이션의 모든 코드에 마이그레이션해야하는 거대한 작업이 될 수 있다는 것에 동의하고, 이 PR에서 해결될 필요는 없다고 생각합니다. 그러나 개인적으로는 차후에라도 도입하는게 좋다고 생각합니다. Eslint의 경우에는 한줄 개행의 포매팅을 전혀 해주지 못해, PR 올릴떄의 불필요한 Diff가 생기는 것을 막지 못합니다. 또한 한줄 개행/그렇지 않음으로 인한 일관성이 깨지는 문제가 존재합니다.

<div>
  고양이
</div>
<div>

  고양이

</div>
<div>

  고양이
</div>
<div>
  고양이

</div>

위 4개의 달라지는 코드 스타일을 ESLint상에서 자동으로 잡지 못하기 때문입니다.