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

build: upgrade node to v20 #413

Merged
merged 3 commits into from
Dec 18, 2023

Conversation

kyoyadmoon
Copy link
Member

@kyoyadmoon kyoyadmoon commented Dec 12, 2023

Purpose

upgrade Node to v20

Changes

  • upgrade webpack to v5
  • upgrade webpack loaders
  • replace node-sass with sass

@@ -5,3 +5,4 @@ es/
lib/
public/
!.storybook/
packages/core/src/icons/components
Copy link
Member Author

Choose a reason for hiding this comment

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

這邊的 components 是 generate-icon-components 產生的

upgrade webpack to v5
upgrade webpack loaders
replace node-sass to sass
@@ -1,21 +1,31 @@
/* eslint-disable import/no-extraneous-dependencies */
const path = require('path');
const autoprefixer = require('autoprefixer');
const ExtractTextPlugin = require('extract-text-webpack-plugin');
const MiniCssExtractPlugin = require('mini-css-extract-plugin');
Copy link
Member Author

Choose a reason for hiding this comment

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

更新 webpack 到 v5 之後,原本的 extract-text-webpack-plugin 不能用了,換成 mini-css-extract-plugin

Comment on lines +25 to +28
library: {
name: toCamelCase(packageName),
type: 'var',
},
Copy link
Member Author

Choose a reason for hiding this comment

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

這裡的 packageName 在新版的 webpack 會被當作變數名稱,原本的值 gypcrete-imageeditor' 會包含 -` 而不合法,所以這邊新增一個 toCamelCase 來轉格式

Comment on lines +52 to +78
use: [
{
loader: MiniCssExtractPlugin.loader,
},
{
loader: 'css-loader',
options: {
importLoaders: 1,
},
{
loader: 'postcss-loader',
options: {
plugins: () => [autoprefixer],
},
{
loader: 'postcss-loader',
options: {
postcssOptions: {
plugins: [autoprefixer],
},
},
{
loader: 'sass-loader',
options: {
},
{
loader: 'sass-loader',
options: {
sassOptions: {
outputStyle: 'expanded',
},
},
],
}),
},
],
Copy link
Member Author

Choose a reason for hiding this comment

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

loader 的 API 參數格式更新

Comment on lines +52 to +62
postcssOptions: {
plugins: [autoprefixer],
},
},
},
{
loader: 'sass-loader',
options: {
outputStyle: 'expanded',
sassOptions: {
outputStyle: 'expanded',
},
Copy link
Member Author

Choose a reason for hiding this comment

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

loader 的 API 參數格式更新

Copy link

codecov bot commented Dec 12, 2023

Codecov Report

Merging #413 (1ccf9e5) into project/upgrade-react-v18 (0f13c76) will not change coverage.
The diff coverage is n/a.

❗ Current head 1ccf9e5 differs from pull request most recent head e068001. Consider uploading reports for the commit e068001 to get more accurate results

Additional details and impacted files
@@                    Coverage Diff                     @@
##           project/upgrade-react-v18     #413   +/-   ##
==========================================================
  Coverage                      88.47%   88.47%           
==========================================================
  Files                            152      152           
  Lines                           1544     1544           
  Branches                         283      283           
==========================================================
  Hits                            1366     1366           
  Misses                           178      178           

Copy link
Contributor

@leannechen leannechen left a comment

Choose a reason for hiding this comment

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

讚 👍🏼

另外好奇會不會考慮加上 .nvmrc?
因為 package.json 的 engines.node 不是強制性的,且只會在安裝時吐警告,對安裝的人的才有提示作用
對開發 gypcrete 的人,如果 local machine 沒有切換到對的 Node version(比如預設是 cloud-frontend 的 v16,但臨時要開發 gypcrete)的話,他就會在 install 甚至 start 的時候才會發現問題

https://10up.github.io/Engineering-Best-Practices/nodejs/

Additionally, it is recommended to create a .nvmrc file with the officially supported node version of the project. This allows those using nvm to run nvm use to switch the node version. We also recommend setting up shell integration to automatically switch node versions when changing directories.

p.s. engine-strict 看起來已經被 npm, yarn 放生了,所以可能不用考慮
npm/cli#786

@@ -7,7 +7,7 @@
"repository": "iCHEF/gypcrete",
"license": "Apache-2.0",
"engines": {
"node": ">= 8.2.1 <= 12",
"node": ">= 20",
Copy link
Contributor

@leannechen leannechen Dec 18, 2023

Choose a reason for hiding this comment

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

在想要不要指定 minor version
現在這樣好處是不用特別做事就能適用到最新的 minor verison
最近有遇到 cloud-frontend 的 Node 版本跟 lib requirement 差一個小版號,所以必須要人工再去處理升版的問題,這時候就覺得指定大版號為止就會省下這個工

雖然不知道會不會因為 minor 不同就有意外的行為不同,不過想像應該還好
Node 跟 Apollo 比起來應該有信用一點 XD

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
Contributor

Choose a reason for hiding this comment

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

同意
那應該是我那邊的調整要指定到大版號就好

@kyoyadmoon
Copy link
Member Author

kyoyadmoon commented Dec 18, 2023

讚 👍🏼

另外好奇會不會考慮加上 .nvmrc? 因為 package.json 的 engines.node 不是強制性的,且只會在安裝時吐警告,對安裝的人的才有提示作用 對開發 gypcrete 的人,如果 local machine 沒有切換到對的 Node version(比如預設是 cloud-frontend 的 v16,但臨時要開發 gypcrete)的話,他就會在 install 甚至 start 的時候才會發現問題

https://10up.github.io/Engineering-Best-Practices/nodejs/

Additionally, it is recommended to create a .nvmrc file with the officially supported node version of the project. This allows those using nvm to run nvm use to switch the node version. We also recommend setting up shell integration to automatically switch node versions when changing directories.

p.s. engine-strict 看起來已經被 npm, yarn 放生了,所以可能不用考慮 npm/cli#786

@leannechen 感謝建議,我覺得如果新增 .nvmrc 可以搭配你上次分享的 shell script 自動切換 node 版本會滿讚的
另外想確定一下 engines.node 不是強制性的意思,因為我在不符合規定版本嘗試執行指令的時候是會被錯誤檔下來的,應該算是有強制性?
我的想法是畢竟也不一定每台機器都有使用 .nvmrc,如果是考慮到版本檢查的話,感覺應該還是透過 engines.node 的設定來做一些檢查會比較通用
CleanShot 2023-12-18 at 15 12 56

Update: 我剛剛發現用 yarn 的話會阻擋,但用 npm run 就不會 😅

@leannechen
Copy link
Contributor

leannechen commented Dec 18, 2023

@kyoyadmoon

不是強制性的意思

先前有印象 as an app 的時候沒有被阻擋下來,不過再實驗了一次確實是 yarn 會擋、是 npm 不會,感謝提醒 🙏🏼
啊然後不是說要拔掉 engines.node 的意思,是除了 package.json 這邊以外,.nvmrc 也有指定的話可能會更好的意思
想像會開發到這些 repo 的人,應該大部分是會用 nvm 的前端
自動切換也可以省力~

--
Update 其他發現
npm 會在 start 後才出現 syntax error,且無法直接知道是與版本有關
例如
CleanShot 2023-12-18 at 15 54 40

Copy link

sonarcloud bot commented Dec 18, 2023

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@kyoyadmoon
Copy link
Member Author

@leannechen 感謝建議,已在 ee89e86 加上 .nvmrc 的 node 版本指定

@kyoyadmoon kyoyadmoon merged commit 08a2eee into project/upgrade-react-v18 Dec 18, 2023
3 checks passed
@kyoyadmoon kyoyadmoon deleted the feat/upgrade-node-v20 branch December 18, 2023 10:08
@zhusee2 zhusee2 added this to the v7 milestone Feb 2, 2024
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

4 participants