Incidental Complexity: Dependency injection is getting in the way

[Gozala]

@hugomrdias @achingbrain we have discussed this a bit on the call, but I don't think I did good job explaining exactly what I meant and comments like this https://github.com/ipfs/js-ipfs/pull/3365/files/fa7c2c13c43bab922052104fe8aff863d9725e9b?file-filters%5B%5D=.js&file-filters%5B%5D=.json&file-filters%5B%5D=.ts#r520398194 prompted me to do actual write up.

If I captured what @achingbrain told js-ipfs in the past used to use more object oriented approach in a sense that there was a class containing all the APIs where each function would just access things it needed on the instance and do it's job. Then at some point things were refactored to the current style.

I don't know the full history so I'm guessing that problem with OOP style was fairly common that is so many things initialized at various times making it really difficult to see what depends on what and what is the order in which things need to be initialized.

Guessing again it was improved by moving to the dependency injection style used today where individual functions are assembled by passing set of components they depend upon, which makes it clear what depends upon what and makes order in which initialization should happen a lot more clear.

However my argument is that:

1. Current approach introduces complexity when component A depends on component B.

   Which is not theoretical, there were bunch of TODO comments that chore: make IPFS API static (remove api-manager) #3365 fixes by breaking components further apart.

2. All this dependency injection just tends to introduce indirection, in practice every single function just needs to call other function, but it does not happens to exists until component creates it and threads it through.

What I would like to suggest is radically simplify this by decoupling (mutable) state from functions. Here is concrete example to put all this into perspective:

• ipfs.add is created from addAll
  

  js-ipfs/packages/ipfs-core/src/components/add.js

  Lines 5 to 18 in 583503b

  	module.exports = ({ addAll }) => {
  	/**
  	* Import a file or data into IPFS.
  	*
  	* @param {Source} source
  	* @param {AddOptions & AbortOptions} [options]
  	* @returns {AddResult}
  	*/
  	async function add (source, options) { // eslint-disable-line require-await
  	/** @type {UnixFSEntry} - Could be undefined if empty */
  	const result = (await last(addAll(source, options)))
  	return result
  	}
  	return add

• Which is created by init
  

  js-ipfs/packages/ipfs-core/src/components/init.js

  Line 150 in 583503b

  const addAll = Components.addAll({ block, preload, pin, gcLock, options: constructorOptions })

• Which in turns requires other components
  https://github.com/ipfs/js-ipfs/blob/master/packages/ipfs-core/src/components/add-all/index.js#L18-L27

• I want go into all of them but pin is interesting because, addAll doesn't really need whole pin API just an pin.add function:
  

  js-ipfs/packages/ipfs-core/src/components/add-all/index.js

  Lines 162 to 165 in 583503b

  	await pin.add(file.cid, {
  	preload: false,
  	lock: false
  	})

Now what I'm proposing is to do is following instead:

add-all.js

const pin = require('./pin/add')

async function addAll(state:IPFSState, source:FileStream, options?:AddAllOptions & AbortOptions):AsyncIterable<UnixFSEntry> {
  // ...
}

add.js

async function add (state:IPFState, source:FileSource, options:AddOptions & AbortOptions) {
  return (await last(addAll(state, source, options)))
}

pin/add.js

const last = require('it-last')
const addAll = require('./add-all')

async function add(state:IPFSState, path:CID|string, options:AddOptions & AbortOptions) => {
  return await last(addAll(state, { path, ...options }, options)) 
}

pin/index.js

const add = require('./add')
const  addAll = require('./add-all')
const  ls = require('./ls')
const  rm = require('./rm')
const rmAll = require('./rm-all')


class PinAPI {
   constructor(state:IPFSState) {
     this.state = state
   }
   add(cid, options) {
     return add(this.state, cid, options)
   }
   // ....
}

Now above example skips ton of details but here are the points it tries to make:

1. Base line APIs can just be static functions that take everything as arguments, which addresses the whole dependency injection and circular dependency problem as they just can call each other.

2. High level APIs (and clusters of them) can be just sugar that delegates to the low level functions.

3. Hypothesis is that IPFSState does not need to contain all that much. In practice I think it's just libp2p , repo and gcLock.

4. If certain component need to maintain state that is not a problem they could have something like init() => State which on node init will be stored under a property in IPFSState.

Not only this has potential to simplify things, but it also can make all the components capable of pick up configuration changes. E.g. right now config is read at startup and changes are ignored by components, which makes sense because reading config on each API call would be costly. However by centralizing a state it becomes trivial for config changes to update that state which all the other components can lookup at call site.

P.S.: I'm aware that libp2p is tied to config as well so it's not going to solve everything, but step at a time.
P.P.S: I'm not suggesting to start this effort now, just a discussion.

[hugomrdias]

i like this! Specially the sub system wrapper class. One note, the http-client does a bunch of factory functions just to support atomic requires of subsystems we can simplify there too!

[achingbrain]

API methods are currently wrapped in withTimeoutOption to handle creation of abort controllers - is there a good way to still have that abstracted away and also generate sane types?

[Gozala]

API methods are currently wrapped in withTimeoutOption to handle creation of abort controllers - is there a good way to still have that abstracted away and also generate sane types?

Sure there is not much changing in that regard, here is the sketch from one of my explorations:

const Preloader = require('../preload')
const { normalizeCidPath } = require('../utils')
const withTimeoutOption = require('ipfs-core-utils/src/with-timeout-option')
const Exporter = require('ipfs-unixfs-exporter')

/**
 * @param {State} state
 * @param {CID|string} ipfsPath
 * @param {CatOptions} [options]
 */
async function * cat (state, ipfsPath, options = {}) {
  const path = normalizeCidPath(ipfsPath)
  if (options.preload !== false) {
    const [base] = path.split('/')
    Preloader.preload(state, base)
  }

  const file = await Exporter.export(state, path, options)
  // File may not have unixfs prop if small & imported with rawLeaves true
  if (file.unixfs && file.unixfs.type.includes('dir')) {
    throw new Error('this dag node is a directory')
  }

   if (!file.content) {
     throw new Error('this dag node has no content')
   }

   yield * file.content(options)
}

module.exports = withTimeoutOption(cat)

Which then in IPFS call is used like:

const { cat } = require('./cat')

class IPFS {
  // ...
  /**
   * @param {CID|string} ipfsPath
   * @param {CatOptions} [options]
   */
   cat(ipfsPath, options) {
      return cat(this.state, ipfsPath, options)
   }
}

[whizzzkid]

js-ipfs is being deprecated in favor of Helia. You can #4336 and read the migration guide.

Please feel to reopen with any comments before 2023-06-05. We will do a final pass on reopened issues afterward (see #4336).

This might already be resolved in Helia, if not please feel free to create an issue there.