fix: move mfs cmds and safer exit (#1981)

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 <mail@hugodias.me>

src/cli/bin.js

@@ -3,79 +3,65 @@
 /* eslint-disable no-console */
 'use strict'
 
-process.on('uncaughtException', (err) => {
-  console.info(err)
-
-  throw err
+// Handle any uncaught errors
+process.once('uncaughtException', (err, origin) => {
+  if (origin === 'uncaughtException') {
+    console.error(err)
+    process.exit(1)
+  }
 })
-
-process.on('unhandledRejection', (err) => {
-  console.info(err)
-
-  throw err
+process.once('unhandledRejection', (err) => {
+  console.error(err)
+  process.exit(1)
 })
 
 const semver = require('semver')
 const pkg = require('../../package.json')
-
+// 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)
 }
 
 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')
 
-function main (args) {
-  const oneWeek = 1000 * 60 * 60 * 24 * 7
-  updateNotifier({ pkg, updateCheckInterval: oneWeek }).notify()
-
-  const cli = new YargsPromise(parser)
-
-  // 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()
+    }
+  })

src/cli/commands/id.js

@@ -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)
     })())
   }
 }

src/cli/parser.js

@@ -1,6 +1,7 @@
 'use strict'
 
 const yargs = require('yargs')
+const mfs = require('ipfs-mfs/cli')
 const utils = require('./utils')
 
 const parser = yargs
@@ -38,4 +39,7 @@ const parser = yargs
   .strict()
   .completion()
 
+// add MFS (Files API) commands
+mfs(parser)
+
 module.exports = parser

src/core/components/files-regular/get-pull-stream.js

@@ -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')
 
@@ -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])

test/cli/config.js

@@ -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')
     })
 
     it('get a config key value', () => {

test/cli/id.js

@@ -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}')
   })
-}))
+})