Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

fix: move mfs cmds and safer exit #1981

Merged
merged 13 commits into from
Aug 1, 2019
112 changes: 49 additions & 63 deletions src/cli/bin.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,79 +3,65 @@
/* eslint-disable no-console */
'use strict'

process.on('uncaughtException', (err) => {
console.info(err)

throw err
})

process.on('unhandledRejection', (err) => {
console.info(err)

throw err
})

const semver = require('semver')
const pkg = require('../../package.json')

if (!semver.satisfies(process.versions.node, pkg.engines.node)) {
console.error(`Please update your Node.js version to ${pkg.engines.node}`)
process.exit(1)
}

const YargsPromise = require('yargs-promise')
const updateNotifier = require('update-notifier')
const { print } = require('./utils')
const mfs = require('ipfs-mfs/cli')
const debug = require('debug')('ipfs:cli')
const parser = require('./parser')
const commandAlias = require('./command-alias')
const { print } = require('./utils')
const pkg = require('../../package.json')

function main (args) {
const oneWeek = 1000 * 60 * 60 * 24 * 7
updateNotifier({ pkg, updateCheckInterval: oneWeek }).notify()
// Handle any uncaught error
process.once('uncaughtException', (err, origin) => {
if (origin === 'uncaughtException') {
print(err.message)
debug(err)
}
})
process.once('unhandledRejection', (err) => {
print(err.message)
debug(err)
})
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

to do that it's better to just let them throw and remove the handlers all together

Copy link
Member

Choose a reason for hiding this comment

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

Screenshot 2019-07-23 at 14 00 14

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 😉


const cli = new YargsPromise(parser)
// Check for node version
if (!semver.satisfies(process.versions.node, pkg.engines.node)) {
console.error(`Please update your Node.js version to ${pkg.engines.node}`)
process.exit(1)
}
Copy link
Member

Choose a reason for hiding this comment

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

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.


// add MFS (Files API) commands
mfs(cli)
// Check if an update is available and notify
const oneWeek = 1000 * 60 * 60 * 24 * 7
updateNotifier({ pkg, updateCheckInterval: oneWeek }).notify()

let getIpfs = null
const cli = new YargsPromise(parser)

// Apply command aliasing (eg `refs local` -> `refs-local`)
args = commandAlias(args)
let getIpfs = null

cli
.parse(args)
.then(({ data, argv }) => {
getIpfs = argv.getIpfs
if (data) {
print(data)
}
})
.catch(({ error, argv }) => {
getIpfs = argv && argv.getIpfs
// Apply command aliasing (eg `refs local` -> `refs-local`)
const args = commandAlias(process.argv.slice(2))
cli
.parse(args)
.then(({ data, argv }) => {
getIpfs = argv.getIpfs
if (data) {
print(data)
}
})
.catch(({ error, argv }) => {
getIpfs = argv.getIpfs
if (error.message) {
print(error.message)
debug(error)
// the argument can have a different shape depending on where the error came from
if (error.message || (error.error && error.error.message)) {
print(error.message || error.error.message)
} else {
print('Unknown error, please re-run the command with DEBUG=ipfs:cli to see debug output')
}
process.exit(1)
})
.finally(async () => {
// If an IPFS instance was used in the handler then clean it up here
if (getIpfs && getIpfs.instance) {
try {
const cleanup = getIpfs.rest[0]
await cleanup()
} catch (err) {
debug(err)
process.exit(1)
}
}
})
}

main(process.argv.slice(2))
} else {
print('Unknown error, please re-run the command with DEBUG=ipfs:cli to see debug output')
debug(error)
}
process.exit(1)
})
.finally(() => {
if (getIpfs && getIpfs.instance) {
const cleanup = getIpfs.rest[0]
return cleanup()
}
})
hugomrdias marked this conversation as resolved.
Show resolved Hide resolved
3 changes: 2 additions & 1 deletion src/cli/commands/id.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ module.exports = {
argv.resolve((async () => {
const ipfs = await argv.getIpfs()
const id = await ipfs.id()
argv.print(JSON.stringify(id, '', 2))

return JSON.stringify(id, '', 2)
})())
}
}
4 changes: 4 additions & 0 deletions src/cli/parser.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
'use strict'

const yargs = require('yargs')
const mfs = require('ipfs-mfs/cli')
const utils = require('./utils')

const parser = yargs
Expand Down Expand Up @@ -38,4 +39,7 @@ const parser = yargs
.strict()
.completion()

// add MFS (Files API) commands
mfs(parser)

module.exports = parser
3 changes: 2 additions & 1 deletion src/core/components/files-regular/get-pull-stream.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const exporter = require('ipfs-unixfs-exporter')
const toPullStream = require('async-iterator-to-pull-stream')
const errCode = require('err-code')
const pull = require('pull-stream/pull')
const pullError = require('pull-stream/sources/error')
const map = require('pull-stream/throughs/map')
const { normalizePath, mapFile } = require('./utils')

Expand All @@ -17,7 +18,7 @@ module.exports = function (self) {
try {
pathComponents = normalizePath(ipfsPath).split('/')
} catch (err) {
return pull.error(errCode(err, 'ERR_INVALID_PATH'))
return pullError(errCode(err, 'ERR_INVALID_PATH'))
}

self._preload(pathComponents[0])
Expand Down
2 changes: 1 addition & 1 deletion test/cli/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Copy link
Member

Choose a reason for hiding this comment

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

I think this test is meant to fail!

Copy link
Member Author

Choose a reason for hiding this comment

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

and still does!
if we don't close the quotes the stdin keeps open for manual input

})

it('get a config key value', () => {
Expand Down
52 changes: 35 additions & 17 deletions test/cli/id.js
Original file line number Diff line number Diff line change
@@ -1,25 +1,43 @@
/* eslint-env mocha */
'use strict'

const expect = require('chai').expect
const runOnAndOff = require('../utils/on-and-off')
const sinon = require('sinon')
const chai = require('chai')
const dirtyChai = require('dirty-chai')
const expect = chai.expect
const YargsPromise = require('yargs-promise')
const clearModule = require('clear-module')
chai.use(dirtyChai)

describe('id', () => runOnAndOff((thing) => {
let ipfs

before(function () {
this.timeout(60 * 1000)
ipfs = thing.ipfs
describe('id', () => {
let cli
let cliUtils
beforeEach(() => {
cliUtils = require('../../src/cli/utils')
cli = new YargsPromise(require('../../src/cli/parser'))
})
afterEach(() => {
sinon.restore()
// TODO: the lines below shouldn't be necessary, cli needs refactor to simplify testability
// Force the next require to not use require cache
clearModule('../../src/cli/utils')
clearModule('../../src/cli/parser')
})

it('get the id', function () {
this.timeout(60 * 1000)
it('should output formatted json string', async () => {
const fakeId = sinon.fake.returns(
{ id: 'id', publicKey: 'publicKey' }
)

sinon
.stub(cliUtils, 'getIPFS')
.callsArgWith(1, null, { id: fakeId })

// TODO: the lines below shouldn't be necessary, cli needs refactor to simplify testability
// Force the next require to not use require cache
clearModule('../../src/cli/commands/id.js')
const { data } = await cli.parse('id')

return ipfs('id').then((res) => {
const id = JSON.parse(res)
expect(id).to.have.property('id')
expect(id).to.have.property('publicKey')
expect(id).to.have.property('addresses')
})
expect(data).to.eq('{\n "id": "id",\n "publicKey": "publicKey"\n}')
})
}))
})
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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