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: SmartHR UIコンポーネントからuseThemeの依存をなくす #4975

Merged
merged 14 commits into from
Oct 10, 2024

Conversation

misako0927
Copy link
Contributor

@misako0927 misako0927 commented Oct 3, 2024

関連URL

https://smarthr.atlassian.net/browse/SHRUI-1052

概要

各コンポーネントがuseThemeに依存していることで、React Server ComponentでSmartHR UIのコンポーネントを使用できない問題を解消したい。

変更内容

  • tailwindConfigをproviderに依存しない形でexportした
  • useThemeを参照していた箇所をtailwindConfigを直接参照するように変更した

確認方法

  1. pr.pkg.newで作られたパッケージをsandbox環境で使用する
  2. tailwindoConfigをNext環境で読み込んで、use clientがなくてもエラーにならないことを確認する
  3. iconなどコンポーネント内でhooksを使用していない、かつuseThemeを元々使用していたコンポーネントがuse clientなしで実行できることを確認する

Copy link

pkg-pr-new bot commented Oct 3, 2024

Open in Stackblitz

pnpm add https://pkg.pr.new/kufu/smarthr-ui@4975

commit: 3961f50

@misako0927 misako0927 changed the title chore: useThemeを参照しないようにする chore: SmartHR UIコンポーネントからuseThemeの依存をなくす Oct 3, 2024
@misako0927 misako0927 marked this pull request as ready for review October 3, 2024 06:04
@misako0927 misako0927 requested a review from a team as a code owner October 3, 2024 06:04
@misako0927 misako0927 requested review from Qs-F, masa0527, s-sasaki-0529 and hiroki0525 and removed request for a team and masa0527 October 3, 2024 06:04

import presetConfig from '../../smarthr-ui-preset'

export const tailwindConfig = resolveConfig(presetConfig)
Copy link
Collaborator

Choose a reason for hiding this comment

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

すべて tailwlindConfig.theme を読んでるので theme を返してしまっても良さそうに思いました。

Copy link
Collaborator

Choose a reason for hiding this comment

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

使う側は import { colors } from '../../themes' とかなるイメージ。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

やってみたのですが、全部を展開できなかったので、今の時点でSmartHR UIで参照している変数のみ出してみたのですがどうでしょうか・・?(ちょっと悩ましいなぁとは思っていますが動的なexportができないので😢)
15cacda

Copy link
Collaborator

Choose a reason for hiding this comment

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

SmartHR UIで参照している変数のみ出してみた

プロダクト側で拡張する場合は、自前で resolveConfig するはずなので問題がないように思いました。smarthr-ui として export する必要もないかも?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

theme自体を返すのをやめた&resolveConfig結果はexport消しました!
4ce49ee

onDelete && onDelete(item)
onChangeSelected &&
if (onDelete) onDelete(item)
if (onChangeSelected)
Copy link
Contributor

Choose a reason for hiding this comment

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

あれ、ここフォーマッタ差分だと思いますが、 CI で lint:format で Prettier によるチェックをしてるはずなのに、変更前後どっちもCI通るのなんでだろ。

Copy link
Contributor

@s-sasaki-0529 s-sasaki-0529 Oct 7, 2024

Choose a reason for hiding this comment

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

あ、そもそもフォーマット差分じゃないのか。
&& だったのを if に変えたんですね?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

そうですね。。
Expected an assignment or function call and instead saw an expression.eslint[@typescript-eslint/no-unused-expressions](https://typescript-eslint.io/rules/no-unused-expressions)
のエラーになっていたので、直しました🙏
katanaでもエラーになって修正を入れていたのでルール変わったのかな〜と思っていたのですが環境問題・・・??

Copy link
Contributor

Choose a reason for hiding this comment

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

あー、なんか pnpm の何かがおかしいのか使い方間違ってるのか、 typescript-eslint のバージョンが v8 になったり v7 になったりすることあるんですよね…。僕の環境だけだと思ったけど普通に起こるのか。さっさと v8 に統一したい。

Copy link
Contributor

Choose a reason for hiding this comment

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

↑ sandbox で使用してる ESLint 周りのバージョン差異のせいっぽかった。
のでこの変更がなくてもCIは通ると思うんですが、まぁ害はない変更だし、いずれは必要になる対応なので良さそう。

Copy link
Contributor

@s-sasaki-0529 s-sasaki-0529 left a comment

Choose a reason for hiding this comment

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

👍️ 現バージョンでは Icon コンポーネントを RSC で利用できなかったのが、利用できるようになることを確認しました。

また、プロダクト側にインストールしてみて、特にデグレが検知されないことも確認しました。

s-sasaki-0529

This comment was marked as duplicate.

Copy link
Contributor

@hiroki0525 hiroki0525 left a comment

Choose a reason for hiding this comment

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

同じくIconがRSCとして使えることを確認しました! :arigatau:

Copy link
Collaborator

@uknmr uknmr left a comment

Choose a reason for hiding this comment

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

GoGo!

@s-sasaki-0529
Copy link
Contributor

をマージしたので、masterを取り込むとちゃんとCIが落ちて Icon が使えるようになったことが検知できてるか確認していただけると助かります!

@s-sasaki-0529
Copy link
Contributor

え! Input/Checkbox/TextLink が RSC できる判定になってる!
https://app.circleci.com/pipelines/github/kufu/smarthr-ui/22414/workflows/991210d1-aa5d-412e-8df1-ba699c6e7db6/jobs/77511

おもいっきり useEffect 使ってるけど、forwardRef 使ってるから実際にレンダリングしないとエラーにならないっぽい…。

@s-sasaki-0529
Copy link
Contributor

別途判定方法を見直しますので、いったん CI 通るように sandbox/next/e2e/rsc.test.ts 修正でお願いします 🙏 🙏 🙏

@misako0927 misako0927 merged commit c341d76 into master Oct 10, 2024
11 checks passed
@misako0927 misako0927 deleted the SHRUI-1052-remove-useTheme branch October 10, 2024 12:36
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.

4 participants