-
Notifications
You must be signed in to change notification settings - Fork 237
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 version checking #2142
Node version checking #2142
Conversation
0df5738
to
a3ebbd8
Compare
current output:
|
We'll do a warning for now, we can see if that works for users. An error may be a breaking change so we can consider that in v14 |
output when using Node 12:
|
output when using Node 14
|
We've found that the kit runs in v14 but not older (due to optional chaining). So we suggest 2 outcomes: If the major Node version is v14, have a warning:
If it is older, have an error (Exit 0 to avoid the nasty stack trace)
|
903480e
to
082dcb4
Compare
seems to work well for |
Use Then make sure you are testing the version of node you want (I'm using nvm) Then run the create command using the packed version of the kit and specifying the version as the packed version |
just to record our findings so far, on create: node 12 stops with an error (as expected) |
082dcb4
to
622d8f1
Compare
the error came from a wrong command, not any of this work, this looks good |
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.
Approved as Joe thinks it looks ok.
bin/cli
Outdated
const isStage2Migrate = argv.command === 'migrate' && process.argv[3] === '--' | ||
const isStage2Create = argv.command === 'init' | ||
|
||
// Do not display error or warning messages when this is stage2 of create or migrate |
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.
What's the reason for making exceptions for "stage 2"? I'm nervous about this because it puts the argument parser into a scope where it needs to run on node versions which we don't support. Anything above the process.exit
line needs to be much more widely compatible than the rest of our codebase.
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.
This was to prevent the warning message being displayed twice. If you have a better way to prevent this, it would be great.
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'd like to use a flag to suppress the message - we can check for that with simpler code which will be more backwards compatible and less likely to break in the future as there is no require
before the version check. For example this line works in the latest version of node v4: !!process.argv.find(x => x === '--suppress-node-version-warning' )
.
The argv parser is likely to get more complex and I don't think it's good to have to stick to older syntax in that file.
just to say this will need a changelog - I'd say it's a fix as it's not a new feature |
ed98d85
to
6ad98eb
Compare
I have made some updates and the output is below for various versions. I've picked the latest of each version. I've gone right back just to show how the errors differ and to see whether we want to improve the messaging for these earlier versions. I have proved locally that I can show the error message on every version back to Node 0.8 (the earliest I can install on my machine) but that requires a code change I'd rather not make and I assume that there's no need to improve the messaging for such old versions. Node 19
Node 18
Node 16
Node 14
Node 12
Node 10
Node 8
Node 6
Node 4
Node 2
Node 0
|
nice, though I think we lost some content - the warning line should be:
Personally I don't think we should care about Node 6 and before issues - thats very old Needs a changelog entry |
Major changes since I approved it and unit tests are failing
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 you've changed the message for warning and error - they are slightly different messages
"Some features may not work with your version." only applies to the warning
Thanks @joelanman I've implemented that now. The output is as follows: Node 19
Node 14
Node 8
|
thanks, just needs a changelog |
I'll approve once there is a changelog as Joe requested |
7d7f8c8
to
5b623ae
Compare
Adding a check for node version before running the kit. Content needs reviewing (@joelanman).