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対応コンポーネント・非対応コンポーネントを機会的に洗い出して担保できる仕組みを導入する #4985

Merged
merged 14 commits into from
Oct 9, 2024

Conversation

s-sasaki-0529
Copy link
Contributor

@s-sasaki-0529 s-sasaki-0529 commented Oct 8, 2024

関連URL

Jira
https://smarthr.atlassian.net/browse/SHRUI-1011

概要

SmartHR UI では、Next.js 向けのサーバーコンポーネント(RSC)として可能な限り使用できるように現在取り組んでいます。

その中で、どのコンポーネントがRSCとして使えるかを機会的に洗い出したり、RSCで使えたものが突然使えなくなるデグレを避けるために、RSCとして使える・使えないを自動テストで担保する仕組みが必要だと考えました。

そこで、E2Eテストの形式で、Next.js 上でサーバーコンポーネントとして各コンポーネントをインポートした際に、エラーが発生する・しないをテストできるようにします。

変更内容

  • Next.js のサンドボックス内に、smarthr-ui が提供するコンポーネントそれぞれに依存したページを作成する
  • Playwright を利用して、それぞれのページ(コンポーネント) にアクセスし、エラーが発生するかしないかを検証するE2Eテストを追加する
  • テストのCIワークフローを作成する

確認方法

ローカルではやや面倒ですが、以下の手順で確認できます。

依存パッケージの更新

# ~/smarthr-ui
$ pnpm install

smarthr-ui 側のビルドとローカルパッケージング

# ~/smarthr-ui/packages/smarthr-ui
$ npx yalc publish

sandbox 側でローカルパッケージの追加と開発環境の起動

# ~/smarthr-ui/sandbox/next
$ npx yalc add smarthr-ui
$ pnpm dev

(開発環境は起動したまま) E2E テストの実行

# ~/smarthr-ui/sandbox/next
$ pnpm playwright test

CI では以下のようなジョブが動いてます。
https://app.circleci.com/pipelines/github/kufu/smarthr-ui/22380/workflows/4b957fe9-8d8e-41f2-b7c9-a47d53190cb8/jobs/77375

Copy link

pkg-pr-new bot commented Oct 8, 2024

Open in Stackblitz

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

commit: 1273817

@@ -32,7 +32,7 @@ commands:
- run: |
sudo corepack enable
sudo corepack prepare
pnpm ui install
pnpm install
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 パッケージだけでなく sandbox まで含めて全部インストールします。
sandbox は他のジョブでは不要なため無駄は生じますが、どのみちキャッシュが CircleCI に残るので問題はなさそう。

- run:
name: build and store smarthr-ui package
working_directory: packages/smarthr-ui
command: npx yalc publish
Copy link
Contributor Author

Choose a reason for hiding this comment

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

現状のSandboxだと、以下みたいに pnpm workspace 使って、開発中の smarthr-ui パッケージを取り込んでます。

"smarthr-ui": "workspace:*"

ただ pnpm の仕様なのか Next の仕様なのか定かでないんですが、このやり方だと

import { Text } from 'smarthr-ui'

とだけ読み込んでも、ツリーシェイキングとか関係なくエントリーファイル(index.ts) からすべてのモジュールが読み込まれてしまって、一つでもクライアントコンポーネントがあるとその時点でアウトという事象が発生してしまいます。。

ので、yalc を使ってローカルにパッケージをビルドします。yalc ビルド版を使うことで、上記のようなコードを書いた場合には意図通りに Text コンポーネントのみが読み込まれ、サーバーコンポーネントとして利用できます。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CI で動かす分には手順通りに常に実行してくれるから良いんですが、ローカルで試したいときにこの手順が煩雑すぎるので、 "smarthr-ui": "workspace:*" のままでも RSC を動かせるソリューション募集中です。

smarthr-ui/lib または smarthr/esm を指定してもダメだった。

Copy link
Contributor

@hiroki0525 hiroki0525 Oct 8, 2024

Choose a reason for hiding this comment

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

下記の2つの方法を試してみると良いかもです

  1. optimizePackageImportssmarthr-ui を追加

バレルファイルはツリーシェイキングに対応していないみたいで、自前で optimizePackageImports を設定して上げる必要があるっぽい?

  1. pnpm build && pnpm start でサーバーを起動する

pnpm dev でNext.jsのサーバーを起動していると思うのですが、上記のコマンドでプロダクション環境と同等のサーバーを起動するとツリーシェイキングが効くかも?

Copy link
Contributor Author

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

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 8, 2024

Choose a reason for hiding this comment

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

んあー、惜しいですね…。

const nextConfig = {
  experimental: {
    optimizePackageImports: ['smarthr-ui'],
  },
}

で、確かに開発サーバー起動後に、RSC対応コンポーネントのページを開いてもそのモジュールしか読み込まれずにRSCレンダリングに成功するんですが、その後RSC非対応コンポーネントのページを開くと useEffect とかが読み込まれてしまって、以降は RSC対応コンポーネントのページを開いてもエラーが出続けます…。

エラーが出ることを正常としてる本PRのユースケースがハックすぎるだけで、このオプション自体は有用そうですね!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pnpm build のほうは、ビルド時点で、RSCで useEffect とかへの依存があることがエラーとして報告されてしまうので、こちらは残念ながら駄目そうです。

working_directory: sandbox/next
command: |
pnpm playwright install chromium
pnpm playwright test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

やや惰性で Playwright を使った E2E テストにしてます。
現状存在する testcafe を用いた E2E テストは近いうちに廃止するつもりです。

ただ本PR時点での使い方だと、Playwright を使うほどでもない気がする。Vitest でも済ませられそうだし極論 curl してステータスコード見るだけでも完結しそう。

/**
* サーバーコンポーネントとして利用できるコンポーネント一覧
*/
const SERVER_COMPONENTS = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

今のところサーバーコンポーネントとして動作するコンポーネントたち。
これを増やしていくのが今期の目標。(なお残り3ヶ月)

/**
* サーバーコンポーネントでは利用できないコンポーネント一覧
*/
const CLIENT_COMPONENTS = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

サーバーコンポーネントでは動かないコンポーネントたち。

仕様上クライアントコンポーネントで当然なものと、工夫次第でサーバーコンポーネントにできるものが混在してるのでそこは整理したい。

Comment on lines +106 to +124
test.describe('RSC対応コンポーネントがRSCで利用できること', () => {
for (const component of SERVER_COMPONENTS) {
test(component, async ({ page }) => {
await page.goto(`http://localhost:3000/rsc_test/${component}`)
await expect(page.getByText(`Success: ${component}`)).toBeVisible()
await expect(page.getByText('Server Error')).not.toBeVisible()
})
}
})

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(`Success: ${component}`)).not.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 として使えるかどうかをテストしてるシナリオ。

エラーが出る=RSCに対応していないというのを検証するのはなかなか尖ったアプローチだなとは思ってます。コスト重視で言えば、RSC対応コンポーネントをすべて列挙したページを1個用意して、そこがレンダリングできればOKぐらいでも良いと思います。

が、「本当はRSCで使えるのに気づかれず使えない扱いされている」という場面を、少なくとも今期中は見落としたくないので、コストを払ってでも確実にチェックするようにしてます。将来的にはもっとシンプルにしても良いかも。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

コンポーネントごとに test を作らずに test.step でステップ分けすれば一々ブラウザコンテキストを作り直さずに済むので高速なんですが、テストの視認性や安定性を考慮して今の形にしました。

"@types/node": "^20.16.10",
"@types/react": "^18.3.11",
"@types/react-dom": "^18.3.0",
"eslint-config-next": "^14.2.14",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

やや別件なんですが、 pnpm install でモノレポ内のすべてのパッケージをインストールした結果、sandbox と smarthr-ui それぞれで ESLint まわりのバージョン不整合が発生してました。

ちゃんとバージョン管理する必要があるんだろうなぁと思いつつ、現状では sandbox に対してリンターを書けたい理由は特にないので削除しちゃいます。

locale: 'ja-JP',
timezoneId: 'Asia/Tokyo',
},
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

適当にプロダクト側から持ってきて調整した Playwright の設定。

export default function AccordionPanelPage() {
console.log(AccordionPanel)
return <div>Success: AccordionPanel</div>
}
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 で動くかを試すためのページ。

use client を指定してないのでこのページはサーバーコンポーネントになります。
各コンポーネントは実際に描画せずとも、import してログ出力するだけで依存が発生するので、RSCで使えるかの識別ができます。

- run:
name: Setup Next.js sandbox
working_directory: sandbox/next
command: npx yalc add smarthr-ui
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": "file:.yalc/smarthr-ui" みたいにローカルビルドしたバージョンがインストールされます。

@@ -0,0 +1,6 @@
import { FaAddressBookIcon } from 'smarthr-ui'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ここだけちょっと個別対応。 Icon というコンポーネントはないため、代表して FaAddressBookIcon を使ってます。流石にアイコンごとに使える使えないが変わるということはないと思うので。

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対応されるので、マージされたらテストが落ちるようになるはず。

@s-sasaki-0529 s-sasaki-0529 marked this pull request as ready for review October 8, 2024 02:37
@s-sasaki-0529 s-sasaki-0529 requested a review from a team as a code owner October 8, 2024 02:37
@s-sasaki-0529 s-sasaki-0529 requested review from misako0927 and uknmr and removed request for a team October 8, 2024 02:37
e2e-test:
executor: node-active-lts-browsers
working_directory: ~/repo
resource_class: large
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Next.js の開発サーバーを起動してリクエストを受け取りまくるって構造の都合上、large (4CPU) を使います。ややお高いですが、デカいプロダクトが何個もある org 全体で見たら誤差程度なので…。

@s-sasaki-0529 s-sasaki-0529 requested a review from a team October 8, 2024 02:45
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.

素敵!

Copy link
Collaborator

Choose a reason for hiding this comment

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

ask: DropdownTrigger や DropdownContent など、組み合わせて使う想定のコンポーネントは対象に含めなくて大丈夫ですか?
Dialog にもありそう。
どこまでやるかの決めだと思いますが、Table や TabBar など展開されてるものとそうでないものの違いが気になりました。

Copy link
Contributor Author

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

Choose a reason for hiding this comment

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

全コンポーネントを対象にする気でいて、同じくStorybook リライトでも全コンポーネントやるだろうからと Story 充足基準シート のほうからコピペしちゃったんですが、こちらも網羅しているわけではなかったですかね…?

Copy link
Collaborator

Choose a reason for hiding this comment

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

あああ、なるほど。Story 充足基準シートでは管理の都合上、一部名寄せしています:pray:
親コンポーネントとしては網羅されてるんですが、Composite して使うコンポーネントに関しては不完全です。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

なるほどー。Storybook は親単体で使うことはありえないってパターンはありますが、今のRSC の検証としては全てをチェックしないといけないので調整が必要ですね。

一旦このPRでは仕組みを作ったとこまででマージして、リストの手直しを別途行います!

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.

3 participants