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

Node.js v22 対応 #1057

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Node.js v22 対応 #1057

wants to merge 8 commits into from

Conversation

YouheiNozaki
Copy link

@YouheiNozaki YouheiNozaki commented Jan 13, 2025

関連:#1025

Node.jsのv22対応を行いました。主要導線やDeepLink、学習分析の反映など動作確認済みです。

@YouheiNozaki YouheiNozaki self-assigned this Jan 13, 2025
@YouheiNozaki YouheiNozaki changed the title Upgrade Node.js v22 LTS2 Node.js v22 対応 Jan 14, 2025
Copy link
Author

Choose a reason for hiding this comment

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

差分はformatの改行のみです。

@@ -9,7 +9,7 @@ jobs:
ref: ${{ github.head_ref }}
- uses: actions/setup-node@v3
with:
node-version: "lts/*"
node-version: "22.13.0"
Copy link
Author

Choose a reason for hiding this comment

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

LTSの最新のバージョンに固定するように修正しました(ローカル環境のNode.jsとバージョンを固定化するため)が、問題あればコメントください。

Copy link

Choose a reason for hiding this comment

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

ローカル環境のNode.jsとバージョンが違うことで問題を生む可能性がありそうですね
そういう点で固定化するのは良いと思います 👍
あるいはこういうとき別の対処として package.json あるいは .nvmrc や .node-version-file を指定してワークツリーの目立つところのに配置して他のツールのサポート(e.g. nvm、mise)を受けられるようにしてみるのもいいかも
一度ご検討くださいませ

具体例:

    - uses: actions/setup-node@v4
      with:
        node-version-file: package.json
:

https://github.com/actions/setup-node/blob/main/docs/advanced-usage.md#node-version-file

Copy link
Author

Choose a reason for hiding this comment

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

ファイル指定できること知見でした!ありがとうございます。package.jsonを参照する形に修正しました

@@ -235,6 +235,9 @@ function countTimeRange(
log.endMs < range.endMs &&
log.endMs - range.startMs > ACTIVITY_COUNT_INTERVAL_THRESHOLD_MS)
) {
if (range.count === undefined) {
Copy link
Author

Choose a reason for hiding this comment

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

Node.js v22関係なくおそらくlintが落ちていたようなので、修正しました(他のCIが通ることを検証したかったため)。

@YouheiNozaki YouheiNozaki requested a review from kou029w January 16, 2025 01:05
Copy link

@kou029w kou029w left a comment

Choose a reason for hiding this comment

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

良いと思います 👍
引き続き他の依存関係の更新(e.g. Moodle、Postgres等コンテナイメージ、npmパッケージ)もお願いしたい (本PRの範疇ではないかと思うのであくまで任意、別PRで構いません) です

Comment on lines 238 to 240
if (range.count === undefined) {
range.count = 0;
}
Copy link

Choose a reason for hiding this comment

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

一瞬だけ検討してみるくらいでOKですがこのように短く書いても読みやすい(個人差ありなので任意)かも

Suggested change
if (range.count === undefined) {
range.count = 0;
}
range.count ??= 0;

Copy link
Author

Choose a reason for hiding this comment

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

修正しました!

@@ -9,7 +9,7 @@ jobs:
ref: ${{ github.head_ref }}
- uses: actions/setup-node@v3
with:
node-version: "lts/*"
node-version: "22.13.0"
Copy link

Choose a reason for hiding this comment

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

ローカル環境のNode.jsとバージョンが違うことで問題を生む可能性がありそうですね
そういう点で固定化するのは良いと思います 👍
あるいはこういうとき別の対処として package.json あるいは .nvmrc や .node-version-file を指定してワークツリーの目立つところのに配置して他のツールのサポート(e.g. nvm、mise)を受けられるようにしてみるのもいいかも
一度ご検討くださいませ

具体例:

    - uses: actions/setup-node@v4
      with:
        node-version-file: package.json
:

https://github.com/actions/setup-node/blob/main/docs/advanced-usage.md#node-version-file

@YouheiNozaki
Copy link
Author

他のライブラリの更新などもこのPRに含めて対応させていただきます。

@YouheiNozaki
Copy link
Author

ひとまずここまで一度ご確認頂いてもよろしいでしょうか?

その他のnpm関連のパッケージは差分大きそうなのでPR分けます🙏
(Next.jsメジャーアップデートなど影響大きそうなのもあるので)

@YouheiNozaki YouheiNozaki requested a review from kou029w January 19, 2025 16:13
Copy link

@kou029w kou029w left a comment

Choose a reason for hiding this comment

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

ありがとうございます!良いと思います 👍

@ties-mitsuhashi
Copy link

b9225d7 コミット時点で以下の大まかな動作を確認し,問題ありませんでした。

  • ブックを作成し,3つのビデオ形式でトピックを作成
  • 配信して学生が見られるか確認
  • 学習分析で作成したトピックの視聴が記録されてるか確認
  • syslogが記録されているか確認
  • DeepLink接続できるか確認

ビルド環境は以下のとおりです。

  • node: v22.13.0
  • npm: 11.0.0
  • yarn 1.22.22

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.

3 participants