-
Notifications
You must be signed in to change notification settings - Fork 32
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
[LEOP-247]Upgrade node to v16 #120
Conversation
packages/react-scripts/package.json
Outdated
@@ -9,7 +9,7 @@ | |||
}, | |||
"license": "MIT", | |||
"engines": { | |||
"node": "^10.12.0 || >=12.0.0" | |||
"node": ">=14" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary for us to declare this package as being only node 14 and above?
I understand we are not using ensure-node-env so this will only produce a warning when being installed.
My understanding though is since this is being released as 9.6.0 - we expect users of node 12 and future adopters of node 16 to be using this.
Since node-sass 6.0.1 can be used on node 12, is it sufficient to leave the node engine as ^10.12.0 || >=12.0.0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary for us to declare this package as being only node 14 and above? I understand we are not using ensure-node-env so this will only produce a warning when being installed. My understanding though is since this is being released as 9.6.0 - we expect users of node 12 and future adopters of node 16 to be using this.
Since node-sass 6.0.1 can be used on node 12, is it sufficient to leave the node engine as
^10.12.0 || >=12.0.0
?
Thanks @michael-booth for your suggestion, I think we should remove '^10.12.0' and set as '>=12.0.0', because node-sass 6.0.1 can't be used on node 10, how do you think of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes agreed @SophiaChen2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
|
||
return nodeSass.types.String(buffer.toString('base64')); | ||
}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The best practice is to one blank line at the end of JS files.
Change list:
Upgrade node to 16
Implement sassFunction
Remove the dependency of bpk-mixins
Upgrade node-sass to V6