-
Notifications
You must be signed in to change notification settings - Fork 312
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: pnpmを使う #2511
chore: pnpmを使う #2511
Conversation
🚀 プレビュー用ページを作成しました 🚀 更新時点でのコミットハッシュ: |
macはビルドが(おそらく運悪く)落ちてるけど、ビルド済みパッケージはアップロードされてそう! |
のチェック項目を全部試した感じ、全く問題なさそうでした!!!!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
変更している箇所は問題なさそうなことを確認しました!
ほぼLGTMです!!!
あと変更漏れがなさそうかチェックしたいと思います(というメモ)
READMEの変更も必要そう!
あと何箇所かwarningが出てそう!(その箇所にコメントできない)
昔からあったワーニングがなぜか発動するようになった感じが・・・ 😇
- name: Setup pnpm | ||
uses: pnpm/action-setup@v4 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(このプルリクエストに関係ないですが)
あれ、この辺り.github/actions/setup-environment/action.yml
使えば不要になる・・・?
なんで共通化してないんだろう。絶対理由があるはずだけど覚えてない。。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 おそらく、共通化したのがテストを速くするための目的で、ビルドも共通化することが目的ではなかったため
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
macOSのインストーラー版とかも試してみた感じ、問題無さそうでした!!
自信を持って進めそう・・・!!
console.log使っちゃダメwarning出てるのは、no-console
の対象範囲をsrc
ディレクトリ内に限定すればとりあえずOKな気がしてます!
(あとそれに伴ってoverrides
内のno-console
条件も変える感じで)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
たぶんあと一点だけ!
残しといていただければ後ほどこちらでやります🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
一箇所だけ変更したあと、アナウンスissueを作ってマージしたいと思います!!
@@ -33,7 +32,8 @@ | |||
"electron:serve": "cross-env VITE_TARGET=electron vite", | |||
"browser:serve": "cross-env VITE_TARGET=browser vite", | |||
"browser:build": "cross-env VITE_TARGET=browser vite build", | |||
"postinstall": "npm run postinstall:packages && npm run postinstall:download-scripts", | |||
"preinstall": "npx -y only-allow pnpm", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 これにより間違えてnpm i
してしまった時にエラーが出る
README.md
Outdated
# 初回のみ | ||
npm i -g pnpm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これは上のドキュメントと統合したらちょっと読みやすくなるかも?
ちょっとこっちで勝手にコミットします!
…o pr/sevenc-nanashi/2511
内容
pnpmをつかうようにします。
関連 Issue
スクリーンショット・動画など
(なし)
その他
(なし)