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

feat: node to version 18 #867

Merged
merged 1 commit into from
Dec 24, 2023
Merged

feat: node to version 18 #867

merged 1 commit into from
Dec 24, 2023

Conversation

yossydev
Copy link
Member

@yossydev yossydev commented Dec 13, 2023

firebase tools v13.0.0以降からNode.js v16以下をサポートしなくなりました。

現状は、firebase toolsのバージョンを下げて対応(#866) していますが、本来はNode.js側を上げるべきなのでこのPRでそちらの対応を行いました!

Copy link
Contributor

github-actions bot commented Dec 13, 2023

Visit the preview URL for this PR (updated for commit 07ec70e):

https://ameba-spindle-hooks--pr867-feat-node-18-n0xb4pcg.web.app

(expires Sun, 21 Jan 2024 09:51:47 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 7dd86a3c25a55dbb81dd4fa259473bf50648140b

@yossydev
Copy link
Member Author

./dist/SnackBar/SnackBar.mjsがバンドルサイズ 1.4KBを超えてしまって落ちている。
diffを取ってnode16と18でのファイルの差分をチェックする

@yossydev yossydev force-pushed the feat/node-18 branch 4 times, most recently from 50bd418 to 6d0ca01 Compare December 13, 2023 10:30
@@ -35,7 +35,7 @@ jobs:
ref: 'refs/pull/${{ github.event.number }}/merge'
- uses: actions/setup-node@v4
with:
node-version: 16
node-version-file: 'package.json'
Copy link
Member Author

Choose a reason for hiding this comment

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

他全部package.jsonで設定しているnodeのバージョンを見てくれているのに、このjobだけ16が使われてしまってCiが落ちている。
ref: https://github.com/openameba/spindle/actions/runs/7193920900/job/19593351619?pr=867#logs

Copy link
Member Author

Choose a reason for hiding this comment

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

いい解決方法が浮かばなかったので、package.jsonのengineの指定を削除するようにしました...

@@ -18,7 +18,7 @@
},
{
"path": "./dist/SnackBar/*.mjs",
"maxSize": "1.4 kB"
"maxSize": "1.5 kB"
Copy link
Member Author

Choose a reason for hiding this comment

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

#867 (comment) の件の対応です。

node.js18と16で出力されたファイルの結果に差分はないのに、npx bundlesizeで落ちてしまっていました。

  • node18: 1.41KB
  • node16: 1.4KB

ライブラリ側の中の計算方法変わったりしたことが原因と予想しています。

こちらの対応として、maxSize1.5にまで上げました。一応1.41でもサイズ指定は可能ですが、そのような指定をしている箇所はなさそうだったので、1.5にしています

Copy link
Contributor

github-actions bot commented Dec 19, 2023

Visit the preview URL for this PR (updated for commit 07ec70e):

https://ameba-spindle--pr867-feat-node-18-a9t26kju.web.app

(expires Sun, 21 Jan 2024 09:53:23 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: e7521619a2dd5c653490c8246e81ec2a5c8f1435

@yossydev yossydev self-assigned this Dec 19, 2023
@yossydev yossydev marked this pull request as ready for review December 19, 2023 06:35
@yossydev yossydev changed the title feat: node to vesrion 18 feat: node to version 18 Dec 19, 2023
@herablog
Copy link
Member

対応ありがとうございます!ちなみに・・version 20じゃないのは何か意図ありま..すか!? (firebase-toolsはこれで良さそうですが、最終的にLTSにしておきたいのもありまして)

@yossydev
Copy link
Member Author

yossydev commented Dec 19, 2023

対応ありがとうございます!ちなみに・・version 20じゃないのは何か意図ありま..すか!? (firebase-toolsはこれで良さそうですが、最終的にLTSにしておきたいのもありまして)

あ、段階的に上げて行った方が良いかなと思って一旦18にしていました!(20にしても動く気はしますが、念の為って感じです👀)
とくに20系を詳しく確認したわけではないです...🙏🏻

yarn.lock Outdated Show resolved Hide resolved
@@ -4,9 +4,6 @@
"main": "./dist/index.js",
"module": "./dist/index.mjs",
"types": "./dist/index.d.ts",
"engines": {
Copy link
Member

Choose a reason for hiding this comment

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

ただ気になっちゃったけど、これなんでこの指定だったんだろ...

Copy link
Member Author

Choose a reason for hiding this comment

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

これめちゃくちゃ謎なんですよね...
一個じゃなくて複数あるのもの謎で...

Copy link
Member

Choose a reason for hiding this comment

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

テストもStorybookの動作も平気そうだからいってみるか〜

- run: yarn install --frozen-lockfile
- run: yarn build:example
- name: deploy to preview channel
uses: FirebaseExtended/action-hosting-deploy@v0
with:
repoToken: "${{ secrets.GITHUB_TOKEN }}"
firebaseServiceAccount: "${{ secrets.FIREBASE_SERVICE_ACCOUNT }}"
repoToken: '${{ secrets.GITHUB_TOKEN }}'
Copy link
Member

Choose a reason for hiding this comment

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

この差分はformatterかな?nodeの変更には直説関連はないけど、悪影響はなさそうだからいいか〜

Copy link
Member Author

@yossydev yossydev Dec 20, 2023

Choose a reason for hiding this comment

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

含めない方がいいかなと思ったんですが、今後も修正するたびに個々の差分出るかと思うので、ここで一緒に修正しちゃっていいかなと思って含めました!👀

@herablog
Copy link
Member

コミットタイプは chore でも良さそうだけど、生成物変わるし CHANGELOG に入れたいとかありますかな

@yossydev
Copy link
Member Author

コミットタイプは chore でも良さそうだけど、生成物変わるし CHANGELOG に入れたいとかありますかな

あれ、これって僕にCHANGELOGに入れたいかって質問ですかね...??👀
であれば、chore: node.js to version 18ってコミットにまとめて、CHANGELOGに含めるというのがいいかなと思ってますがいかかでしょうか...??👀

@herablog
Copy link
Member

あれ、これって僕にCHANGELOGに入れたいかって質問ですかね...??👀

です!

CHANGELOG 自動で生成してまして、feat fix だけ入るのでCHANGELOG入れたいかどうかで決めてよく、今のままで良いか(意図的か)の確認です!

@yossydev
Copy link
Member Author

CHANGELOG 自動で生成してまして、feat fix だけ入るのでCHANGELOG入れたいかどうかで決めてよく、今のままで良いか(意図的か)の確認です!

あーなるほどです!!featfixのコミットのみ、CHANGELOGで自動生成される際に入るって感じなんですね!!:kanzenrikai:
であれば自分は今のままで大丈夫です!!

feat: node to vesrion 18 ← にコミット全てまとめましたー!

Copy link
Member

@herablog herablog left a comment

Choose a reason for hiding this comment

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

エンジニアを褒めるネコ:いいね

@yossydev yossydev merged commit b08ba42 into main Dec 24, 2023
11 checks passed
@yossydev yossydev deleted the feat/node-18 branch December 24, 2023 06:10
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.

None yet

2 participants