-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
avoid throwing for flow control #1924
Conversation
engines: { | ||
node: '>=12', | ||
}, | ||
})) |
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.
Think its better to hardcode the engines here as well so this doesn't suddenly fail when we no longer support version 17 (probably a little ways off)
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 missed that.
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.
Oh true, the list right now is 12, 14, 16, 17
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.
Avoiding throwing an error can be a good idea 👍
But rather than returning string | null
- which actually I would have expect to be the return type of the validateNodeEngineVersion function- with the string being kinda error message, would it be possible to return something that would look like a result struct which could looks like the following?
{
status: bool,
message: string?
}
Or, just, a boolean, and the caller could be responsible for the error message?
I did consider using a promise as a kind of Maybe object, so the calling code could do something like validateNodeEngineVersion(process.version).catch(errorMessage => {
console.error(error) // eslint-disable-line no-console
process.exit(1)
}) Still using exceptions for flow control really, but perhaps it reads better? Another alternative would be to pass the assertion function a callback to run if the engine is invalid, like: validateNodeEngineVersion(process.version, errorMessage => {
console.error(error) // eslint-disable-line no-console
process.exit(1)
}) That way the conditional stays inside the function. That's probably my favourite of them all. |
@mattwynne I quite like the callback idea there. |
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.
lovely!
It's from an older version of the code.
) | ||
}) | ||
|
||
it('passes when the version is greater than specified in package.json', () => { | ||
assertNodeEngineVersion('v17.0.0') | ||
it('returns null when the version is greater than specified in package.json', () => { |
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 just realised we should have changed these example descriptions too: we're not returning anything anymore.
Instead of having a try/catch around this function just have it return an optional error message.