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

fix(ui): vitest lint 세팅 오류 수정 #126

Merged
merged 3 commits into from
Sep 10, 2024
Merged

Conversation

jungwoo3490
Copy link
Member

@jungwoo3490 jungwoo3490 commented Sep 9, 2024

변경사항

release 워크플로우에서 lint 스크립트 동작 과정에서 문제가 발생했어요.

image

원인은 vitest lint 세팅을 하고자 react-internal.js에 추가했던 부분 때문이었어요.

extends: [
  '@vercel/style-guide/eslint/browser',
  '@vercel/style-guide/eslint/typescript',
  '@vercel/style-guide/eslint/react',
  // 새로 추가한 부분
  '@vercel/style-guide/eslint/vitest,
].map(require.resolve),

패키지 코드에는 분명히 vitest.js가 존재했는데, 알고보니 저희 패키지에 설치된 @vercel/style-guide의 버전은 5.0.0이었고, Vitest eslint rule이 릴리즈된 버전은 6.0.0 버전이어서 오류가 발생했던 것이었어요.

image

그래서 latest 버전으로 업그레이드를 시도했는데, 문제가 많이 발생했어요.

latest 버전으로 업그레이드하게 되면 업그레이드한 패키지가 의존하는 vite 버전과 기존에 설치된 패키지가 요구하는 vite 버전에 충돌이 발생하게 돼요.
lock파일 임의 변경도 금지되어 있어 여러모로 의존성 충돌을 해결하기 어려운 상황이라 vitest가 사용되는 ui 패키지 내부의 eslint 파일에서 직접 rule을 세팅해주기로 했어요.

4달 전 eslint-plugin-vitest라는 플러그인이 개발되어서 이걸 사용해서 lint 세팅을 해주었어요.

// packages/ui/eslintrc.js

const vitest = require('eslint-plugin-vitest');

module.exports = {
  extends: ['custom/react-internal'],
  plugins: ['vitest'],
  rules: {
    ...vitest.configs.recommended.rules,
  },
};

그 후 lint 스크립트 동작시켰더니 오류가 발생하지 않았어요.

image

fork한 레포에서 workflow 테스트도 해보았는데 잘 동작하는 것을 확인했어요.

image

추가 작업

  • 기존에 lint rule이 어긋난 부분이 두 군데 있어서 --fix 스크립트로 수정해서 반영했어요.
  • lint 커맨드 결과가 자꾸 캐싱되어서 의도적으로 오류를 발생시켰는데도 이전 성공 로그가 캐싱되어 성공했다고 출력이 되었어요. 사실 lint는 테스트 용도이기 때문에 결과가 캐싱되면 결과 확인에 혼선이 생길 수도 있다고 생각해서 turbo.json에서 cache 옵션을 비활성화시켰어요. 같은 이유로 test 커맨드도 cache 옵션을 비활성화시켰어요.

시급한 정도

🏃‍♂️ 보통 : 최대한 빠르게 리뷰 부탁드립니다.

Copy link

height bot commented Sep 9, 2024

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

Copy link

changeset-bot bot commented Sep 9, 2024

⚠️ No Changeset found

Latest commit: 97a3b4e

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

Copy link
Member

@suwonthugger suwonthugger left a comment

Choose a reason for hiding this comment

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

꼼꼼한 pr 리뷰 감사드립니다. 원인 부터 해결 방법까지 자세히 알게되었어요. @vercel/style-guide 의 버전이 맞지 않아 발생한 문제였군요..! eslint-plugin-vitest 통한 해결 방법 하나 배워갑니다~ 수고많으셨어요!

@Brokyeom
Copy link
Member

고생 많았습니다!! 세팅중에 초기에 세팅하고, 크게 건드리지 않았던 린터 부분이었는데, 잘 찾아서 세팅해주어서 고마워요
곳곳에 린트나 포맷이 맞지 않는 파일들이 존재할텐데, 그때마다 조금씩 개선 해 봅시다!

@Brokyeom Brokyeom merged commit 0723d92 into main Sep 10, 2024
1 check passed
@Brokyeom Brokyeom deleted the fix/vitest-lint-error branch September 10, 2024 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants