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
Merged
32 changes: 31 additions & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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 に残るので問題はなさそう。

- save_cache:
paths:
- node_modules
Expand All @@ -57,6 +57,27 @@ commands:
command: pnpm ui test-storybook:ci
- store_test_results:
path: junit.xml
run-e2e-test:
steps:
- 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 とかへの依存があることがエラーとして報告されてしまうので、こちらは残念ながら駄目そうです。

- 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" みたいにローカルビルドしたバージョンがインストールされます。

- run:
name: Start Next.js sandbox
working_directory: sandbox/next
command: pnpm dev
background: true
- run:
name: Run E2E Test
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 してステータスコード見るだけでも完結しそう。

run-chromatic:
steps:
- checkout
Expand Down Expand Up @@ -100,6 +121,13 @@ jobs:
- setup-for-test
- install-noto-sans-cjk-jp
- run-a11y-test
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 全体で見たら誤差程度なので…。

steps:
- setup-for-test
- run-e2e-test
chromatic-deployment:
executor: node-active-lts
resource_class: medium+
Expand All @@ -121,6 +149,8 @@ workflows:
context: smarthr-dockerhub
- a11y-test:
context: smarthr-dockerhub
- e2e-test:
context: smarthr-dockerhub
- chromatic-deployment:
filters:
branches:
Expand Down
Loading