refactor: remove option normalisation (#454)

There are just too many options, to many variables and too many
different calling contexts so let's get the user to be explicit
about how they intend to use the module rather than try to
second guess them.

BREAKING CHANGE: ipfsd-ctl no longer defaults to a local ipfs, ipfs-http-client or ipfs bin, now the user needs to set those packages when creating controllers and remote controller server.

.aegir.js

@@ -1,8 +1,25 @@
 'use strict'
 
 const createServer = require('./src').createServer
+const { findBin } = require('./src/utils')
 
-const server = createServer() // using defaults
+const server = createServer(null, {
+  ipfsModule: {
+    path: require.resolve('ipfs'),
+    ref: require('ipfs')
+  },
+  ipfsHttpModule: {
+    path: require.resolve('ipfs-http-client'),
+    ref: require('ipfs-http-client')
+  }
+}, {
+  go: {
+    ipfsBin: findBin('go', true)
+  },
+  js: {
+    ipfsBin: findBin('js', true)
+  }
+}) // using defaults
 module.exports = {
   bundlesize: { maxSize: '35kB' },
   karma: {

src/endpoint/routes.js

@@ -20,7 +20,7 @@ const badRequest = err => {
   } else {
     msg = err.message
   }
-  debug(msg)
+  debug(err)
   throw boom.badRequest(msg)
 }
 

src/factory.js

@@ -1,7 +1,7 @@
 'use strict'
 const merge = require('merge-options').bind({ ignoreUndefined: true })
 const kyOriginal = require('ky-universal').default
-const { tmpDir, findBin } = require('./utils')
+const { tmpDir } = require('./utils')
 const { isNode } = require('ipfs-utils/src/env')
 const ControllerDaemon = require('./ipfsd-daemon')
 const ControllerRemote = require('./ipfsd-client')
@@ -77,27 +77,11 @@ class Factory {
     const opts = {
       json: {
         ...options,
-        ipfsModule: undefined,
-        ipfsHttpModule: undefined,
         // avoid recursive spawning
         remote: false
       }
     }
 
-    if (options.ipfsModule && options.ipfsModule.path) {
-      opts.json.ipfsModule = {
-        path: options.ipfsModule.path
-        // n.b. no ref property - do not send code refs over http
-      }
-    }
-
-    if (options.ipfsHttpModule && options.ipfsHttpModule.path) {
-      opts.json.ipfsHttpModule = {
-        path: options.ipfsHttpModule.path
-        // n.b. no ref property - do not send code refs over http
-      }
-    }
-
     const res = await ky.post(
       `${options.endpoint}/spawn`,
       opts
@@ -121,36 +105,6 @@ class Factory {
       options
     )
 
-    // conditionally include ipfs based on which type of daemon we will spawn when none has been specified
-    if ((opts.type === 'js' || opts.type === 'proc') && !opts.ipfsModule) {
-      opts.ipfsModule = {}
-    }
-
-    if (opts.ipfsModule) {
-      if (!opts.ipfsModule.path) {
-        opts.ipfsModule.path = require.resolve('ipfs')
-      }
-
-      if (!opts.ipfsModule.ref) {
-        opts.ipfsModule.ref = require('ipfs')
-      }
-    }
-
-    // only include the http api client if it has not been specified as an option
-    // for example if we are testing the http api client itself we should not try
-    // to require 'ipfs-http-client'
-    if (!opts.ipfsHttpModule) {
-      opts.ipfsHttpModule = {
-        path: require.resolve('ipfs-http-client'),
-        ref: require('ipfs-http-client')
-      }
-    }
-
-    // find ipfs binary if not specified
-    if (opts.type !== 'proc' && !opts.ipfsBin) {
-      opts.ipfsBin = findBin(opts.type, true)
-    }
-
     // IPFS options defaults
     const ipfsOptions = merge(
       {

src/index.js

@@ -32,13 +32,18 @@ const createController = (options) => {
  *
  * @param {(Object|number)} options - Configuration options or just the port.
  * @param {number} options.port - Port to start the server on.
+ * @param {ControllerOptions} factoryOptions
+ * @param {ControllerOptionsOverrides} factoryOverrides
  * @returns {Server}
  */
-const createServer = (options) => {
+const createServer = (options, factoryOptions = {}, factoryOverrides = {}) => {
   if (typeof options === 'number') {
     options = { port: options }
   }
-  return new Server(options, createFactory)
+
+  return new Server(options, () => {
+    return createFactory(factoryOptions, factoryOverrides)
+  })
 }
 
 module.exports = {

src/ipfsd-client.js

@@ -34,8 +34,9 @@ class Client {
     this.disposable = remoteState.disposable
     this.clean = remoteState.clean
     this.api = null
-    this.apiAddr = this._setApi(remoteState.apiAddr)
-    this.gatewayAddr = this._setGateway(remoteState.gatewayAddr)
+
+    this._setApi(remoteState.apiAddr)
+    this._setGateway(remoteState.gatewayAddr)
   }
 
   /**

test/browser.utils.js

@@ -36,7 +36,11 @@ describe('utils browser version', function () {
         test: true,
         type: 'proc',
         disposable: false,
-        ipfsOptions: { repo: 'ipfs_test_remove' }
+        ipfsOptions: { repo: 'ipfs_test_remove' },
+        ipfsModule: {
+          path: require.resolve('ipfs'),
+          ref: require('ipfs')
+        }
       })
       await ctl.init()
       await ctl.start()
@@ -51,7 +55,16 @@ describe('utils browser version', function () {
     describe('repoExists', () => {
       it('should resolve true when repo exists', async () => {
         const f = createFactory({ test: true })
-        const node = await f.spawn({ type: 'proc', ipfsOptions: { repo: 'ipfs_test' } })
+        const node = await f.spawn({
+          type: 'proc',
+          ipfsOptions: {
+            repo: 'ipfs_test'
+          },
+          ipfsModule: {
+            path: require.resolve('ipfs'),
+            ref: require('ipfs')
+          }
+        })
         expect(await repoExists('ipfs_test')).to.be.true()
         await node.stop()
       })

test/controller.spec.js

@@ -6,23 +6,84 @@ const merge = require('merge-options')
 const dirtyChai = require('dirty-chai')
 const chaiPromise = require('chai-as-promised')
 const { createFactory, createController } = require('../src')
-const { repoExists } = require('../src/utils')
+const { repoExists, findBin } = require('../src/utils')
 const { isBrowser, isWebWorker } = require('ipfs-utils/src/env')
 
 const expect = chai.expect
 chai.use(dirtyChai)
 chai.use(chaiPromise)
 
-const types = [
-  { type: 'js', test: true, ipfsOptions: { init: false, start: false } },
-  { type: 'go', test: true, ipfsOptions: { init: false, start: false } },
-  { type: 'proc', test: true, ipfsOptions: { init: false, start: false } },
-  { type: 'js', remote: true, test: true, ipfsOptions: { init: false, start: false } },
-  { type: 'go', remote: true, test: true, ipfsOptions: { init: false, start: false } }
-]
+const defaultOpts = {
+  ipfsModule: {
+    path: require.resolve('ipfs'),
+    ref: require('ipfs')
+  },
+  ipfsHttpModule: {
+    path: require.resolve('ipfs-http-client'),
+    ref: require('ipfs-http-client')
+  },
+  ipfsBin: findBin('js', true)
+}
+
+const types = [{
+  ...defaultOpts,
+  type: 'js',
+  test: true,
+  ipfsOptions: {
+    init: false,
+    start: false
+  }
+}, {
+  ...defaultOpts,
+  ipfsBin: findBin('go', true),
+  type: 'go',
+  test: true,
+  ipfsOptions: {
+    init: false,
+    start: false
+  }
+}, {
+  ...defaultOpts,
+  type: 'proc',
+  test: true,
+  ipfsOptions: {
+    init: false,
+    start: false
+  }
+}, {
+  ...defaultOpts,
+  type: 'js',
+  remote: true,
+  test: true,
+  ipfsOptions: {
+    init: false,
+    start: false
+  }
+}, {
+  ...defaultOpts,
+  ipfsBin: findBin('go', true),
+  type: 'go',
+  remote: true,
+  test: true,
+  ipfsOptions: {
+    init: false,
+    start: false
+  }
+}]
 
 describe('Controller API', () => {
-  const factory = createFactory({ test: true })
+  const factory = createFactory({
+    test: true,
+    ipfsModule: {
+      path: require.resolve('ipfs'),
+      ref: require('ipfs')
+    },
+    ipfsHttpModule: {
+      path: require.resolve('ipfs-http-client'),
+      ref: require('ipfs-http-client')
+    },
+    ipfsBin: findBin('js', true)
+  })
 
   before(() => factory.spawn({ type: 'js' }))
 
@@ -48,7 +109,11 @@ describe('Controller API', () => {
     describe('should work with a initialized repo', () => {
       for (const opts of types) {
         it(`type: ${opts.type} remote: ${Boolean(opts.remote)}`, async () => {
-          const ctl = await createController(merge(opts, { ipfsOptions: { repo: factory.controllers[0].path } }))
+          const ctl = await createController(merge(opts, {
+            ipfsOptions: {
+              repo: factory.controllers[0].path
+            }
+          }))
 
           await ctl.init()
           expect(ctl.initialized).to.be.true()
@@ -130,8 +195,11 @@ describe('Controller API', () => {
             return this.skip() // browser in proc can't attach to running node
           }
           const ctl = await createController(merge(
-            opts,
-            { ipfsOptions: { repo: factory.controllers[0].path } }
+            opts, {
+              ipfsOptions: {
+                repo: factory.controllers[0].path
+              }
+            }
           ))
 
           await ctl.init()
@@ -184,10 +252,7 @@ describe('Controller API', () => {
           const f = createFactory()
           const ctl = await f.spawn(merge(opts, {
             disposable: false,
-            test: true,
-            ipfsOptions: {
-              repo: await f.tmpDir()
-            }
+            test: true
           }))
 
           await ctl.init()