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

依存関係周りを整理 #1989

Conversation

sevenc-nanashi
Copy link
Member

@sevenc-nanashi sevenc-nanashi commented Apr 13, 2024

内容

  • vite、typescript、eslint、electron周りとdeprecated警告が出てたopenapi-generator-cliをアプデします。
  • Nodeを18.20.2、npmを10.5.0に更新します(18.20.2の付属のはず)
  • vite v5の更新に追従します(vite.config.ts -> vite.config.mts
  • ts-nodebabel.config.jscore-jsが使われてなかったので削除します。
  • cross-var でbabel周りの警告が大量(具体的には36 critical)に出てたのでdrop-in replacementらしい cross-replace に置き換えます。
  • license-checkerを、アクティブにメンテされてそうなlicense-checker-rseidelsohnに置き換えます。これによりDeprecated警告が消えます。

関連 Issue

(なし)

スクリーンショット・動画など

image

その他

(なし)

@sevenc-nanashi sevenc-nanashi requested a review from a team as a code owner April 13, 2024 16:10
@sevenc-nanashi sevenc-nanashi requested review from Hiroshiba and removed request for a team April 13, 2024 16:10
"@voicevox/eslint-plugin": "file:./eslint-plugin",
"@vue/eslint-config-prettier": "7.0.0",
"@vue/eslint-config-typescript": "11.0.2",
"@vue/test-utils": "2.3.0",
"cross-env": "7.0.3",
"cross-var": "1.1.0",
"cross-replace": "0.2.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

cross-replace、まだダウンロード数が少ないんですが、

  • (ちゃんと動くこと:確認中)
  • 警告を放置してると警告が増えたことに気がつけない

ので置き換えています。もっと良い解決策があるかも。

Copy link
Member

Choose a reason for hiding this comment

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

修正ありがとうございます!!!

これくらいのコード量ならsubmodule的な感じで自分たちのコードで管理しても良いかもと思いました。
まあ意外とOS差を吸収する必要があって大変なのですが・・・。

そもそも使わないという選択もあるかもです。(個人的には依存減らせるのでこれを支持したい。)
npm runを増設して環境変数で渡すのではなく、--prepackage パスelectron:build_pneverにわたす以前のコードに戻すのはどうでしょう?
44a81fa#diff-5c3fa597431eda03ac3339ae6bf7f05e1a50d6fc7333679ec38e21b337cb6721L685-R669

@@ -5,7 +5,7 @@
"private": true,
"main": "./dist/main.js",
"engines": {
"node": ">=18.13.0 <19",
"node": ">=18.20.2 <19",
Copy link
Member Author

Choose a reason for hiding this comment

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

typescript-eslintが要求してたのであげました。そのうち20にしてもよさそう?

@sevenc-nanashi
Copy link
Member Author

使われてなかったbabel.config.jsと一緒にDockerfileとdocker-compose.ymlも爆破しようと思うんですが、これって使われてます?

@@ -3,7 +3,7 @@
* ランタイム情報には起動しているエンジンのURLなどが含まれる。
*/

import fs from "fs";
import fs from "fs/promises";
Copy link
Member Author

Choose a reason for hiding this comment

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

何故かfsだとエラーを吐かれたのでpromisesに置き換え。

maxBuffer: 1024 * 1024 * 10, // FIXME: stdoutではなくファイル出力にする
}
);
const licenseJson = await new Promise((resolve, reject) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

cmdで呼ぼうとしたら色々面倒なことになったので、APIを使う用に書き換えました。

@sevenc-nanashi
Copy link
Member Author

prettier、色々と変更が入ったらしくレビューが大変そうなのでとりあえずmtsをサポートする最小のバージョン(2.5)まで上げました。別で3.x代に上げるPRを作るかも

@Hiroshiba
Copy link
Member

Hiroshiba commented Apr 15, 2024

使われてなかったbabel.config.jsと一緒にDockerfileとdocker-compose.ymlも爆破しようと思うんですが、これって使われてます?

わかんないですね。issue作って頂けると!!

prettier、色々と変更が入ったらしくレビューが大変そうなのでとりあえずmtsをサポートする最小のバージョン(2.5)まで上げました。別で3.x代に上げるPRを作るかも

配慮助かります!!

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

プルリクエストありがとうございます!!

ちょっと一気にいろいろ変更されているので、申し訳ないのですが小分けにするのをお願いします 🙇
というのもたぶんどれか問題があった場合にrevertをかけたくなるわけですが、変更が多いとかけるにかけられなくなってしまうためです。

コードの変更が生じているものは可能な限り分けたいです。
特にフォーマッターが変わっている?ものはrevert時にでかい確率でコンフリクトが生じるので分けとくと将来の僕たちを助けられるかなと。

あとfs周りが変わっているの(Node更新?)もstore辺りのコードが変わってるので独立させられるなら分離しちゃいたいです。

vite周り・cross-var周り・ライセンス周りもそれぞれ分けたほうが良いと思います。
が、まあこれらは変更頻度が少ないかもなので後回しでも。(あとでまたお願いするかも)

依存関係がもつれてて難しそうだったらなんとかできないか僕も考えます!

@sevenc-nanashi
Copy link
Member Author

なるほど、とりあえずPrettier更新だけで分けようと思います。

@Hiroshiba
Copy link
Member

すみません、助かります🙇

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.

2 participants