Skip to content
This repository has been archived by the owner on Jan 13, 2023. It is now read-only.

Add CI to test apps which use app-common #146

Merged
merged 8 commits into from
Dec 20, 2021

Conversation

d-hayashi
Copy link
Contributor

What?

Add CI to test apps which use app-common

Why?

To avoid destructive changes

@hdl-service hdl-service added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 16, 2021
@d-hayashi d-hayashi changed the title Add CI to test apps which use app-common [WIP] Add CI to test apps which use app-common Dec 16, 2021
@hdl-service hdl-service added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 16, 2021
@d-hayashi d-hayashi requested review from WatanabeToshimitsu and removed request for yusukefs December 16, 2021 10:57
@WatanabeToshimitsu
Copy link
Contributor

ローカル環境でも,app-common の node_modules を消すことで同じエラーを再現できました
なので,現在 CI が上手く動かないのは,app-common の node_modules を消してることに原因があるようです

先に結論を述べると,

  1. app-common の node_modules を残す
  2. app-common の node_modules を消して,アプリ側のビルドツールの設定を弄って,jest によるテストを一旦消す

のどっちかが取れる手段だと思います

これ以上テストを書く障壁を高くしたくないですし,この CI のためにアプリ側のコード弄るのはなんか違う気がするので,個人的には 1. を推してます

以下詳しい説明をば
長くなるし細かいので興味があればで大丈夫です

なんで typescript エラーが出るのか

そもそも,Node は,パッケージの解決をする時に,その js ファイルの実際のパスの node_modules 以下を見て,そこに該当するパッケージがないと,親ディレクトリの node_modules を探す動作を繰り返します
そして,typescript コンパイラはこれを再現する動きをします
また,app-common は MUI 等の幾つかのパッケージを peerDeps としていて,app-common のビルド時にはそれをバンドルせずに,app-common を使うアプリ側のビルド時にそれをバンドルさせるようになっています.つまり,app-common のビルド後のコードには import 文が含まれていて,これを使用アプリケーション側で解決する必要があります

今回の場合,app-common の実際のパスは app-user-manager/node_modules 配下でなく,/app-common にあるので,/app-common/node_modules が無い場合,app-common のビルド後のコードにある import 文を解決出来なくなります
そのせいで,app-common から import した型の内,MUI の型を流用したものが any 扱いになってエラーが出る,という流れです

どうすれば通るのか

typescript のコンパイル実行時に preserveSymlinks を指定するとtypescript のエラーは直ります
このオプションを true にすると,typescript コンパイラはシンボリックリンクの実際のパスを展開せずに,実際のパスがリンク先にあるかのように扱うようになります

ただ,jest と vite,webpack(storybook が使用しています)に関しても同じようなことが起きて,npm run test がどこかで失敗します
vitewebpack には,typescript の preserveSymlinks オプションと同等のものがあるんですが,jest の方がまだ対応中のようで,CI をこのままにテストを通すとなると jest の実行は諦める必要があります(現状あるオプションでは上手く動きませんでした)

また,少なくともローカルでは,app-common の node_modules を消さなければ npm run test が通るので,そのように CI を変えることでも CI が通るはずです

@d-hayashi d-hayashi changed the title [WIP] Add CI to test apps which use app-common Add CI to test apps which use app-common Dec 20, 2021
@hdl-service hdl-service removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 20, 2021
@d-hayashi
Copy link
Contributor Author

@WatanabeToshimitsu
コメントあざます!
ひとまずテストが回るところまでは確認できたので、レビューお願いします!

Copy link
Contributor

@WatanabeToshimitsu WatanabeToshimitsu left a comment

Choose a reason for hiding this comment

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

LGTM!

@hdl-service hdl-service added the lgtm Indicates that a PR is ready to be merged. label Dec 20, 2021
@hdl-service
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: d-hayashi, WatanabeToshimitsu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [WatanabeToshimitsu,d-hayashi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@WatanabeToshimitsu WatanabeToshimitsu merged commit ee75a21 into master Dec 20, 2021
@hdl-service hdl-service deleted the feature/test-apps branch December 20, 2021 11:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants