refactor: [RadioButton]コンポーネントのpropsやstateに依存関係のないものは関数外で定義した #4936
Conversation
hiroki0525
requested review from
misako0927 and
uknmr
and removed request for
a team
commit: |
| @@ -52,18 +52,20 @@ const radioButton = tv({ | |||
| }, | |||
| }) | |||
|
|
|||
| const { wrapper, innerWrapper, box, input, label } = radioButton() | |||
| const innerWrapperStyle = innerWrapper() | |||
| const inputStyle = input() | |||
There was a problem hiding this comment.
[imo]
良いような気もするんですけど、variant 使うかどうかで外と中どっちで定義するか変わってくるのはちょっとややこしいような気もしました。
再レンダリングコストについても、元々 useMemo 内にあるので基本的には再評価されませんしまぁ…。
ただ tailwind-variants 的なお作法の視点での良し悪しはわからない…。
There was a problem hiding this comment.
variant 使うかどうかで外と中どっちで定義するか変わってくるのはちょっとややこしいような気もしました。
tailwind-variants 的なお作法の視点での良し悪しはわからない…。
この辺は確かにあるかもです...うーん
再レンダリングコストについても、元々 useMemo 内にあるので基本的には再評価されませんしまぁ…。
一応厳密にいうと、下記のように再評価のプロセスが変わる認識ですね
before:
useMemoで定義している依存配列に変更radioButtonが評価innerWrapperとinputStyleが評価
after:
useMemoで定義している依存配列に変更radioButtonは評価されない。innerWrapperとinputStyleも評価されない
ただ、まぁ細かいチューニングの類になると思うので、冒頭の可読性との兼ね合いかなぁ...と思います。
今回の変更にめちゃ強い気持ちもないので、他のレビュアーの方の意見も伺いたいですね
There was a problem hiding this comment.
めちゃむずい・・・となってます🤔
今後全部この方針でいくんだ！というのであれば直してもいいと思いますが、今ほぼ全てのコンポーネントでuseMemo内にあったりするので、可読性を考えるとここだけ直すモチベは薄いかなぁと思いましたがどうでしょうか🧐
There was a problem hiding this comment.
一応処理速度上がったが計測したのですが、予想に反して遅くなりました😇
今回対応した関数外に出したパターンが3ms、もともとあった useMemo 内で定義するパターンが一回の useMemo による再評価あたり 0.1 ms前後でした
だいぶモチベも薄くなってきたのでクローズしようと思います！
関連URL
概要
タイトルの通りで、特にコンポーネント内部で定義する必要がないものについてはコンポーネント外で定義しました。
再レンダリング時に再評価されるのを防いだり、直接的にコンポーネントのpropsやstateの依存関係のない静的なものはコンポーネント外で出すことでコードを見やすくするため。
変更内容
マージ後なにも影響はない想定です。
確認方法