fix: move mfs cmds and safer exit #1981
Conversation
ghost
assigned 
|
ping @hugomrdias - any chance you can get this one ready to merge? |
This PR moves mfs cli commands to parser.js to make them available for tests and also add `signal-exit` to the cli to make forced exits safer.
Co-Authored-By: hugomrdias <[email protected]>
hugomrdias
force-pushed
the
fix/fix-mfs-safer-exit
branch
from
5a52c88
to
b9836a2
Compare
| if (!semver.satisfies(process.versions.node, pkg.engines.node)) { | ||
| console.error(`Please update your Node.js version to ${pkg.engines.node}`) | ||
| process.exit(1) | ||
| } |
There was a problem hiding this comment.
The uncaughtException/unhandledRejection handlers and node version check need to be above the require statements as they were because any one of them could cause an error or use code that is not supported in the version of Node.js that the user is running.
| process.once('unhandledRejection', (err) => { | ||
| print(err.message) | ||
| debug(err) | ||
| }) |
There was a problem hiding this comment.
Can we log these out as they were before? ...(maybe console.error not console.info?)
These unexpected errors aren't always easy to reproduce so getting hold of a stack trace after the fact could be difficult and time consuming. You'd have to run the command again in debug mode and somehow replicate the circumstances that trigger the error.
Users also rarely run in debug mode so won't see the stack either. That translates to it being more difficult for us to begin to debug if users open an issue with just an error message and not a stack trace.
It also makes it harder for users to self debug - users unfamiliar with the debug module need to figure out how to turn it on in order to get the stack.
There was a problem hiding this comment.
to do that it's better to just let them throw and remove the handlers all together
There was a problem hiding this comment.
First is with:
process.on('uncaughtException', err => {
console.error(err)
process.exit(1)
})...second is without.
Not much in it TBH. It's kinda nice to have the Node.js internal stack greyed out.
My main concern is having the stack logged for these unexpected errors and not behind a debug flag 😉
| @@ -58,7 +58,7 @@ describe('config', () => runOnAndOff((thing) => { | |||
| }) | |||
|
|
|||
| it('set a config key with invalid json', () => { | |||
| return ipfs.fail('config foo {"bar:0} --json') | |||
| return ipfs.fail('config foo {"bar:0"} --json') | |||
There was a problem hiding this comment.
I think this test is meant to fail!
There was a problem hiding this comment.
and still does!
if we don't close the quotes the stdin keeps open for manual input
| }) | ||
| })) | ||
| }) |
There was a problem hiding this comment.
This is quite complicated, would it be better to just require the command and call the handler, passing in the mocks and asserting on the result?
There was a problem hiding this comment.
yes i know thats one of the things im gonna do next, do injection with https://github.com/yargs/yargs/blob/master/docs/api.md#parseargs-context-parsecallback
this will become just
it('should output formatted json string', async () => {
const { data } = await cli.parse('id', {
ipfs: {
id: Promise.resolve({ id: 'id', publicKey: 'publicKey' })
}
})
expect(data).to.eq('{\n "id": "id",\n "publicKey": "publicKey"\n}')
})
no spies, no mocks, no stubs just plain simple js
This PR moves mfs cli commands to parser.js to make them available for tests and also tweak a bit error handling.