Skip to content

Conversation

aherlihy
Copy link
Contributor

Should we exit or just warn?

Copy link
Collaborator

@mcasimir mcasimir left a comment

Choose a reason for hiding this comment

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

Just requesting a change in case you missed semver.satisfies. Really cool to have this one, will work great also as a smoke test for the release.

Copy link
Contributor

@lrlna lrlna left a comment

Choose a reason for hiding this comment

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

imo this definitely needs to exit instead of warn (so what you have here is good). If, for some reason, somebody's system is on pre node 4.x, they will not be able to do very much in our shell.

console.log(`${clr('Using Mongosh Beta', ['bold', 'yellow'])}: ${version}`);
console.log(`${MONGOSH_WIKI}`);
if (semver.lt(process.version, engines.node.replace('^', ''))) {
console.log(`WARNING: mismatched node version. Minimum node version required ${engines.node}. Currently using ${process.version}. Exiting...\n\n`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe let's throw an MongoshUnsupportedError here?

Message is really good though -- super clear with node versions, hopefully will lead to less bug reports.

Copy link
Contributor Author

@aherlihy aherlihy Jun 15, 2020

Choose a reason for hiding this comment

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

We don't have unsupported, we only have unimplemented, which I think doesn't match this use case. I can change it to RuntimeError

Actually it doesn't make sense to throw an error right before exiting because we would need to catch that error otherwise the process.exit code is unreachable. I moved the check to the constructor so we don't have a writer either. I think it should stay as is.

@lrlna
Copy link
Contributor

lrlna commented Jun 15, 2020

(omg sorry for all the comments on such a tiny PR 😅 😅 😅 )

@aherlihy
Copy link
Contributor Author

Never a problem on the many comments, I opened this PR as a way of starting this conversation :)

@aherlihy aherlihy merged commit 49bc725 into master Jun 15, 2020
@aherlihy aherlihy deleted the node-version branch June 15, 2020 11:23
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.

3 participants