feat!: useId の独自実装を削除し、React 18 未満のサポートを終了する #4920
Conversation
commit: |
| @@ -127,6 +127,3 @@ export { defaultBreakpoint } from './themes/createBreakpoint' | |||
|
|
|||
| // constants | |||
| export { FONT_FAMILY, CHART_COLORS, OTHER_CHART_COLOR } from './constants' | |||
|
|
|||
| // utils | |||
| export { SequencePrefixIdProvider } from './hooks/useId' | |||
There was a problem hiding this comment.
こちらもリポジトリ内及び社内では一切使用されていなかったのであわせて削除してます。
| } | ||
| return <IdContext.Provider value={value}>{children}</IdContext.Provider> | ||
| // eslint-disable-next-line react-hooks/rules-of-hooks | ||
| return React.useId() |
There was a problem hiding this comment.
ここ、 if (defaultId) 分岐の下に Hooks が登場するので、Hooks の制約違反で警告が出てしまうので無視してます。
修正前 (return ('useId' in React ? React.useId : useId_OLD)()) は動的にコードを組み立ててた都合、さすがの ESLint も検知できなかったようです。
コンポーネント内で defaultId を渡すか渡さないかが描画ごとに異なる場合におかしな挙動になる可能性があるなぁと感じますが、これまで異常がなかったので大丈夫かなと思ってます。
There was a problem hiding this comment.
あるいはこうすることで回避するほうが安全かも…？
export const useId = (defaultId?: string): string => {
const id = React.useId()
return defaultId || id
}
There was a problem hiding this comment.
@s-sasaki-0529 上記でいただいているコードのほうが良いと思いました！
There was a problem hiding this comment.
厳密には挙動が変わることになるからためらってたんですが、少なくとも悪化することは無さそうなのでそうしちゃいます！
| "react": "16.13.0 || ^17.0.1 || ^18.0.0", | ||
| "react-dom": "16.13.0 || ^17.0.1 || ^18.0.0", | ||
| "react": "^18.0.0", | ||
| "react-dom": "^18.0.0", |
There was a problem hiding this comment.
事実上、React 17 以下で使えないコンポーネントが生まれることになるのでここも変えちゃってよいですよね。
のでこっちのほうがインパクト強いという意味で、PRタイトルに含めてます。
s-sasaki-0529
requested review from
atzzCokeK and
hiroki0525
and removed request for
a team
| } | ||
| return <IdContext.Provider value={value}>{children}</IdContext.Provider> | ||
| const id = React.useId() | ||
| return defaultId || id |
There was a problem hiding this comment.
#4920 (comment)
この辺のやりとりもあって実装ちょっと調整してます。
There was a problem hiding this comment.
defaultId を渡す機構は必要なのか気になりました。
There was a problem hiding this comment.
プロダクト側からIDを明示したい場合にそれを使用して、指定がなければランダム値を使うみたいな用途っぽいですね。
useId がその機構を持たずに、コンポーネント側に分岐を書けば良いという説はあるし、そのほうが健全な気もします。
There was a problem hiding this comment.
kufu 内を調べてみましたが、useId(someId) の形で使っているのは、smarthr-ui のみ（Checkbox、RadioButton、Switch）でいずれも someId || useId() と同等の使い方でした。
useId() に defaultId を渡した場合に、存在すれば defaultId が返ってくるのは useId の責務ではないと考えられるので、消しても良いと思います。
https://github.com/search?q=org%3Akufu+useId&type=code
仰るとおりなのでやっていきなんですが、PR は分けても良いですか…？ React 17 で使えなくなるのはユーザー影響ありありですが、内部での id の決定方法はユーザー影響を出さないリファクタになるし、検証観点も異なってくるので切り分けて扱いたいです！ |
|
レビューありがとうございました！ |
関連URL
https://smarthr.atlassian.net/browse/SHRUI-1047
概要
React 17 以下にも提供していた独自の
userIdの仕組みを廃止します。これによって事実上 React 18 未満のサポートを終了します。変更内容
SmartHR UI が提供する
useIdは、React 組み込みのuseIdをラップし、一部機能を拡張して提供しています。その中で、useId が存在しない React 18 未満でも使用できるように、同等の機能を独自実装していました。
独自実装では
useContextが使われていますが、これによって、useIdを使用している各種コンポーネントがサーバーコンポーネントとして利用できない問題が発生しています。RSC 対応を進めたいという気持ちと、React 18 未満のサポートはぼちぼち終了しても良さそうという背景から、独自実装を廃止し、React 18 のみをサポートするように変更します。
確認方法
React 18 を使用している場合には影響は一切ありません。
React 17 以下を使用している場合、SmartHR UI を使用することができなくなるため、SmartHR UI のバージョンアップを止めるか、React をバージョンアップしてください。