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: RSCのテスト用ページで、コンポーネントをレンダリングするところまで検証するように修正 #4997

Merged
merged 5 commits into from
Oct 11, 2024

Conversation

s-sasaki-0529
Copy link
Contributor

関連URL

前回PR

概要

前回PRにて、コンポーネントがRSCレンダリング可能かを検証するページを用意してE2Eで担保するようにしたが、検証方法が不十分で判定が正しく出来ていなかったことがわかったため、判定方法を見直す。

変更内容

変更前は、コンポーネントを import して、コンソール出力するだけだった。

これだとモジュールのトップレベルで useContext などの依存が発生しているコンポーネントの場合はその時点でエラーとなるが、コンポーネントをレンダリングするタイミングで初めて useState や useRef に依存する場合のコンポーネントを検知できていなかった。

そこで、本PRではすべてのコンポーネントについて、最小限のレンダリングを行うように修正して、E2Eテストの期待値も見直した。

確認方法

CIが通ればOKです。ローカルで検証する場合は前回PRを参照してください。

@s-sasaki-0529 s-sasaki-0529 requested a review from a team as a code owner October 10, 2024 11:04
@s-sasaki-0529 s-sasaki-0529 requested review from moshisora and hiroki0525 and removed request for a team October 10, 2024 11:04
Copy link

pkg-pr-new bot commented Oct 10, 2024

Open in Stackblitz

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

commit: d9c5c4e

'RemoteDialogTrigger',
'TableReel',
'BulkActionRow',
'Tooltip',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

RSC対応してると思ったが全然してなかったコンポーネントたち。
良くみたらラジオボタンとかダイアログトリガーとかそれはそうって感じでしたね。

@@ -52,7 +52,6 @@ const CLIENT_COMPONENTS = [
'Calendar',
'FormControl',
'Th',
'Cell',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

いくつか既に存在していないコンポーネントが紛れていたので削除。

Copy link
Contributor Author

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

Choose a reason for hiding this comment

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

このリストから漏れてるコンポーネントについては別途追加します。
(間に合ったらこのPRに混ぜますが、次時間取れるときにやるので、構わずレビューしちゃってOKです)

@@ -116,7 +112,7 @@ test.describe('RSC非対応コンポーネントはRSCでエラーになるこ
for (const component of CLIENT_COMPONENTS) {
test(component, async ({ page }) => {
await page.goto(`http://localhost:3000/rsc_test/${component}`)
await expect(page.getByText('Server Error')).toBeVisible()
await expect(page.getByText(/Server Error|Unhandled Runtime Error/)).toBeVisible()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

レンダリングのタイミングでRSC違反が発生した場合は Unhandled Runtime Error になるので期待値を調整。

<div>Success: AccordionPanel</div>
<AccordionPanel />
</>
)
Copy link
Contributor Author

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

Choose a reason for hiding this comment

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

こんな感じにコンポーネントのレンダリングまでやるように。
流石にここまでやってエラーが出なかったらRSC対応してると言って良いでしょう。

onClickAction={() => {}}
onClickClose={() => {}}
isOpen
/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

必須propsがある場合は、それを省略したせいで発生したエラーと区別がつかなくなるので、温かみのある手作業ですべて設定してます。

@s-sasaki-0529 s-sasaki-0529 requested a review from a team October 10, 2024 11:09
@@ -66,7 +65,6 @@ const CLIENT_COMPONENTS = [
'Header',
'NotificationBar',
'TabBar',
'FormGroup',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

いくつか既に存在していないコンポーネントが紛れていたので削除。
このリストから漏れてるコンポーネントについては別途追加します。

@@ -1,6 +1,10 @@
import React from 'react'
Copy link
Contributor

@hiroki0525 hiroki0525 Oct 11, 2024

Choose a reason for hiding this comment

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

[Q]
この一文って必要なんでしたっけ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[A]
現代の React だと不要なやつです!
ただ tsconfig.json と ESLint の設定が必要になるやつなので横着して入れてしまいました。

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.

1個コメントしましたが、特に問題なさそうなのでアプルーブします!
:gogo:

@s-sasaki-0529 s-sasaki-0529 merged commit 80ef81f into master Oct 11, 2024
20 checks passed
@s-sasaki-0529 s-sasaki-0529 deleted the fix-rsc-test-page branch October 11, 2024 08:04
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