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

fix: .npmrcによりpackage.json記載のnodeバージョンに満たない場合はビルドに失敗するようにする #12755

Merged
merged 1 commit into from
Dec 23, 2023

Conversation

samunohito
Copy link
Member

What

package.jsonに記載されているnodeのバージョンに満たない場合、pnpm buildに失敗するようになります。

Why

今回の更新でnodeのバージョンが引き上げられるため、dockerを用いず運用しているサーバ管理者各位が気づきやすくするための施策です。

Additional info (optional)

手元の端末でv18とv20.10.0でpnpm buildを実行し、エラーになること、エラーとならずビルドが動くことをそれぞれ確認

Checklist

  • Read the contribution guide
  • Test working in a local environment
  • (If needed) Add story of storybook
  • (If needed) Update CHANGELOG.md
  • (If possible) Add tests

Copy link

codecov bot commented Dec 23, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2c7d07b) 79.79% compared to head (c0555a3) 81.88%.
Report is 2 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #12755      +/-   ##
===========================================
+ Coverage    79.79%   81.88%   +2.09%     
===========================================
  Files          956      179     -777     
  Lines       108801    27438   -81363     
  Branches      8372      511    -7861     
===========================================
- Hits         86817    22468   -64349     
+ Misses       21984     4970   -17014     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@syuilo syuilo merged commit 1716c65 into misskey-dev:develop Dec 23, 2023
15 checks passed
@syuilo
Copy link
Member

syuilo commented Dec 23, 2023

👍

@tamaina
Copy link
Contributor

tamaina commented Dec 23, 2023

これめちゃくちゃ文句言われた記憶があるんだけど(LTSじゃないからという話もあったが、単純に最新すぎると動かない環境がある)

何らかのAPIやバグのためにv20.10が必要というならそれをREADMEに書いたほうが良さそう

@samunohito
Copy link
Member Author

.npmrcを取っ払えばビルドできるといったら出来るので、やるなら自己責任でやってほしい旨をNoteの冒頭に書き加えればよさそうですかね?

@tamaina
Copy link
Contributor

tamaina commented Dec 23, 2023

そういうことはわざわざ書かなくてもいいのでは…

@tamaina
Copy link
Contributor

tamaina commented Dec 23, 2023

純粋になぜバージョンを上げるのか合理的でなくてもいいので理由がほしい

@yuriha-chan
Copy link
Contributor

最新のLTS以前のバージョンは公式にサポートする意志がなく、動作テストもしていないので、最低バージョンとして必須にしているという意図であれば、それはそのように書くと親切かと思います。

そのように書いた場合でも、最新バージョンのAPIに特に依存しておらず実際には動く場合、「気づきやすくなる」というメリットよりも、余計なおせっかいと受け取られる可能性はあるかとは思います。

@syuilo
Copy link
Member

syuilo commented Dec 23, 2023

依存関係の更新に伴いとかで良さそう

@syuilo
Copy link
Member

syuilo commented Dec 23, 2023

「⚪︎⚪︎ライブラリの⚪︎⚪︎関数がバージョン⚪︎⚪︎からNode 20じゃないと動作しない」とか書いても煩雑だし

@samunohito
Copy link
Member Author

engine-strictによる制限、無い方がいいですか?
そうであれば、revertするプルリクを別途発行します

@syuilo
Copy link
Member

syuilo commented Dec 23, 2023

無くて困ることはあるけど合って困ることはなさそう

@tamaina
Copy link
Contributor

tamaina commented Dec 23, 2023

それはそうなんだけど、マイナーバージョンまで厳密に管理されると困る人がいるかなと

@syuilo
Copy link
Member

syuilo commented Dec 23, 2023

とはいっても厳密に管理しないとMisskey動かないし

@anatawa12
Copy link
Member

LTS矯正するなら>= 20.9.0にするとかが良さそう? 20.xだと非LTS含むし

@syuilo
Copy link
Member

syuilo commented Dec 23, 2023

起動してから動かないことが分かるよりはビルド時に分かった方が嬉しそう

@anatawa12
Copy link
Member

起動してから動かないことが分かるよりはビルド時に分かった方が嬉しそう

これは同意

@tamaina
Copy link
Contributor

tamaina commented Dec 23, 2023

厳密に管理してるなら多少煩雑でもCHANGELOGに書いてほしい気持ちがある

@tamaina
Copy link
Contributor

tamaina commented Dec 23, 2023

というかこれが適用されると↓の方式で導入した環境で動かなくなる気がする

image

https://misskey-hub.net/ja/docs/for-admin/install/guides/ubuntu-manual/#nodejs

@syuilo
Copy link
Member

syuilo commented Dec 23, 2023

このPRに関わらず動かなくはなりそう(依存ライブラリがnode 20.10が必要になったため)(具体的にどのライブラリだったかは忘れた)

@tamaina
Copy link
Contributor

tamaina commented Dec 23, 2023

(具体的にどのライブラリだったかは忘れた)

そういうのめっちゃ重要なので覚えておいてほしい

@tamaina
Copy link
Contributor

tamaina commented Dec 23, 2023

正直p1.a9z.devはまだv20.5.1なので何かしら壊れてるのか

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.

5 participants