-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
fix(cli): Don't warn when running on node 9 #4816
Conversation
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.
I'm okay changing the supported versions constant to include Node 9 but I don't think having it in the CI matrix is a good idea for now.
src/constants.js
Outdated
@@ -13,7 +13,7 @@ export const DEPENDENCY_TYPES = ['devDependencies', 'dependencies', 'optionalDep | |||
export const RESOLUTIONS = 'resolutions'; | |||
export const MANIFEST_FIELDS = [RESOLUTIONS, ...DEPENDENCY_TYPES]; | |||
|
|||
export const SUPPORTED_NODE_VERSIONS = '^4.8.0 || ^5.7.0 || ^6.2.2 || ^8.0.0'; | |||
export const SUPPORTED_NODE_VERSIONS = '^4.8.0 || ^5.7.0 || ^6.2.2 || ^8.0.0 || ^9.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.
I think we can simply do >= 8.0.0
for now to be more future-proof.
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.
Ok, but then we'd need to add that to CI
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.
Not really. It is just to make the warning go away since we simply don't know.
appveyor.yml
Outdated
@@ -1,5 +1,6 @@ | |||
environment: | |||
matrix: | |||
- node_version: "9" |
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.
Since Node 9 will be "experimental" I don't think adding it to the build chain would be a good idea. Things may break or change randomly and that would cause churn with the team and reduce the signal/noise ratio for CI build results.
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.
I'm curios, what do you mean with experimental?
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.
Formally it's stable, not experimental. Experimental were RCs before the final. Node 8 is LTS but that doesn't mean Node 9 is not stable; it officially is.
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.
It's odd to test on support node 5, which has been EOL for over a year, but not node current
This change will decrease the build size from 9.94 MB to 9.94 MB, a decrease of 1.44 KB (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.
Thanks a lot!
**Summary** Update the accepted semver range to not warn on unknown future versions of node. **Test plan** N/A since we don't want to add Node 9 into our build matrix just yet.
Summary
Update the accepted semver range to not warn on unknown future versions of node.
Test plan
N/A since we don't want to add Node 9 into our build matrix just yet.