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

chore: enable rule "useHookAtTopLevel" #37

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

schiughi
Copy link
Contributor

oven-webappでreact hooks をトップレベル以外で使用してもlintエラーになっていないようでした
see usePublicEventByUserStatus

export const usePublicEventByUserStatus = ({
  aliasCode,
  publicEventId,
}: Options) => {
  const { isManager } = useAuthenticatedManager({ aliasCode });

  return isManager
    ? useGetManagedPublicEvent({ aliasCode, publicEventId })
    : useGetPublicEvent({ aliasCode, publicEventId });
};

biomeのrecommendedの設定に useExhaustiveDependencies は含まれているのに対し useHookAtTopLevel が含まれていないのが原因のようです
see https://biomejs.dev/linter/rules/
(☑がついてるやつがrecommendedのもの)
image

React以外のプロジェクトでこのエラーを検知してしまう煩わしさより、Reactプロジェクトで検知漏れが起きるリスクの方が大きそうなのでこちらで対応しました!
何か追加してない理由があれば教えてください 🙇

recommendedに含まれていなかったので追加
see: https://biomejs.dev/linter/rules/
@schiughi schiughi self-assigned this Jan 10, 2025
@schiughi schiughi requested a review from susiyaki January 10, 2025 02:07
Copy link
Collaborator

@susiyaki susiyaki left a comment

Choose a reason for hiding this comment

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

確かにこちらのルールは有効化した方がよさそうです!
サンプルで上げていただいたようなコードは僕も以前気になり、hooksでなくただの関数で定義するか、それぞれのhooksに分割する方がよいというレビューをしていて気になっているところでした 🙇
有効になっていないことに気づいていなかったので防げるようになるとよさそうです!

@susiyaki
Copy link
Collaborator

マージして、ReleasesのDraftに反映されたら「Publish release」を押すとjsrに反映されます 👌
https://github.com/jubilee-works/biome-config/releases

@schiughi
Copy link
Contributor Author

なるほど!ありがとうございます!
やってみます!

@schiughi schiughi merged commit 02fab6c into main Jan 10, 2025
1 check passed
@schiughi schiughi deleted the chore/enable-rule-use-hook-at-top-level branch January 10, 2025 02:20
@schiughi
Copy link
Contributor Author

Publish releaseどこ… 😭

@susiyaki
Copy link
Collaborator

↓のペンのアイコンを押してあげると画面の下部にあります 🙏
image

@schiughi
Copy link
Contributor Author

あった!できました!
ありがとうございます!

@susiyaki
Copy link
Collaborator

susiyaki commented Jan 10, 2025

ありがとうございます!

ちょっとjsrへのpushが認証切れっぽくて落ちていたので確認します!(年が変わったからかな...)
actorNotScopeMemberなのでリポジトリに参加していないからだ...!
https://github.com/jubilee-works/biome-config/actions/runs/12702114854/job/35407762884

@susiyaki
Copy link
Collaborator

@schiughi
jsrの@timetreeに所属している人でないと権限がないようなので、招待するために1度jsrに登録をお願いしてもいいですか?

@schiughi
Copy link
Contributor Author

@susiyaki
お!なるほど!
jsr登録しました!

@susiyaki
Copy link
Collaborator

@schiughi
招待しました!(たぶん次からは行けるはず...!)

@schiughi
Copy link
Contributor Author

ありがとうございますー!:tada:

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.

2 participants