Feature request: Allow extending libp2p.transports/libp2p.discovery instead of overriding it via flag

mkg20001 commented 5 years ago

Version: next
Platform: *
Subsystem: libp2p component

Type: feature

Severity: low

Description: Feature request: Allow extending libp2p.transports/libp2p.discovery instead of overriding it via flag

It would be good if there would be a flag to add custom transports in addition to the defaults, instead of fully overriding them.

This flag could be named libp2p.extendOnly or libp2p.extends

lidel commented 5 years ago

I had to override entire js-libp2p bundle used in Brave's js-ipfs just to add TCP transport. I'd love to simplify this and switch to more granual extension mechanism suggested here.

mkg20001 commented 5 years ago

I'm going to implement this in my current PR

alanshaw commented 4 years ago

Would it be better to pass the defaultOptions to the function so that you can just merge/replace as you please?

const ipfs = await IPFS.create({
  libp2p: ({ defaultOptions, peerInfo }) => {
    defaultOptions.transports.push(...)
    return new Libp2p(defaultOptions)
  }
})

mkg20001 commented 4 years ago

While it does make sense in this use-case, I don't know if we should make libp2p an array and merge the function-results like I did in #2578, since I've now added the ability to add custom transports that have custom option available via the options object as part of the libp2p bundle, so one would need multiple entries, some of which might be objects, to fully control libp2p then.

lidel commented 4 years ago

I think we should keep this very simple and @alanshaw is on the right track.

Right now, if I want to add something before passing config to js-libp2p, I need to override entire defaultBundle.

What if we make a simple change and run custom function creating bundle against defaults from src/core/components/libp2p.js#L136 instead?

Something like this (in /src/core/components/libp2p.js):

module.exports = function libp2p (self, config) {
  const options = self._options || {}
  config = config || {}
-  // Always create libp2p via a bundle function
-  const createBundle = typeof options.libp2p === 'function'
-    ? options.libp2p
-    : defaultBundle
-
   const { datastore } = self._repo
   const peerInfo = self._peerInfo
   const peerBook = self._peerInfoBook
-  const libp2p = createBundle({ options, config, datastore, peerInfo, peerBook })
+  const libp2p = defaultBundle({ options, config, datastore, peerInfo, peerBook })

and later at the end of defaultBundle function:

+  let customBundle
+  if (typeof options.libp2p === 'function') {
+    customBundle = options.libp2p
+    delete options.libp2p
+  }
+
   const libp2pOptions = mergeOptions(libp2pDefaults, get(options, 'libp2p', {}))
+  if (customBundle) {
+    return customBundle({ libp2pOptions, config, datastore, peerInfo, peerBook })
+  }

  // Required inline to reduce startup time
  // Note: libp2p-nodejs gets replaced by libp2p-browser when webpacked/browserified
  const Node = require('../runtime/libp2p-nodejs')
  return new Node(libp2pOptions)

This way customBundle function gets libp2pOptions with defaults and can decide if those should be extended or replaced.

PS. I like the idea of having "drop-in" wrappers like stardust4ipfs, they make high level code easier to read. If we implement the above, those could still exist in the form of:

  libp2p: ({ datastore, peerInfo, peerBook, options, config }) => {
    options = require('stardust4ipfs')(options, config, peerInfo)
    options = require('brave4ipfs')(options, config, peerInfo, datastore, peerBook)
    return new Libp2p(options)
  }

We could add some sugar that enables us to run them sequentially to update config with the most common options:

  libp2p: [require('stardust4ipfs'), require('brave4ipfs')]

mkg20001 commented 4 years ago

We could add some sugar that enables us to run them sequentially to update config with the most common options:

:+1:

options = require('stardust4ipfs')(options, config, peerInfo)

We should instead possibly pass the whole object as-is instead of splitting it up, so you can just directly do libp2p: require('stardust4ipfs') if you only need one extension

We should maybe not do return new Libp2p(options) but instead just return options for better chaining? Or no return at all and let new Libp2p() be handled outside of the "custom bundle function"?

This would also help enabling the libp2p: require('stardust4ipfs') usecase

alanshaw commented 4 years ago

Returning an instance allows you to subclass Libp2p. Please can we just do the simpliest thing and pass the libp2p options to the bundle function so you can extend/replace (like I suggested in https://github.com/ipfs/js-ipfs/issues/2579#issuecomment-549273709 and as @lidel fleshed out in https://github.com/ipfs/js-ipfs/issues/2579#issuecomment-549399564)?

mkg20001 commented 4 years ago

So, I've created something that allows multiple methods of overriding the libp2p bundle

libp2p: Function({ datastore, options, ...})
- can directly modify options.libp2p and return undefined
- can return new options object that either extends or overrides current options.libp2p by setting extend flag on it
- can return a Libp2p instance
libp2p: Object
- can extend the libp2p object (by setting extend property) or override it

All of those can be chained, so one can do

{
  libp2p: [ // any of those is optional
    require('stardust4ipfs'),
    require('brave4ipfs'),
    {extend: true, transports: [MyCustomTransport]},
    ({options: {libp2p}}) => new Libp2p(libp2p)
  ]
}

If none of the Functions returns libp2p instance, then the default Node from ./runtime/libp2p-nodejs.js (or browser) will be called with the current value of options.libp2p

mkg20001 commented 4 years ago

It's pushed in #2578

alanshaw commented 4 years ago

{
  libp2p: [ // any of those is optional
    require('stardust4ipfs'),
    require('brave4ipfs'),
    {extend: true, transports: [MyCustomTransport]},
    ({options: {libp2p}}) => new Libp2p(libp2p)
  ]
}

I get it, but this is too complicated. I really want to get the stardust change in but it should be a small focused change. If we're going to allow configuration like this then we should support it more generally for all the options in the constructor (which is a bigger proposal/change and separate PR and discussion). If you want this convenience right now you can easily create a module that accepts this array and spits out an options => Libp2p function.

mkg20001 commented 4 years ago

Ok. So I'll just do the array function in the curent PR and make another PR with the more complex example later?

mkg20001 commented 4 years ago

The problem I see with @lidel's solution is that while customBundle gets passed the options for the runtime, the actual transports are mixed in inside the runtime not defaultBundle.

The problem with that is, that we only get access to the result of defaultBundle and not the runtime. The problem if we'd even had access would be that while the runtime supports adding custom transports, due to the fact it uses mergeOptions as well it doesn't extend but instead override those transport arrays in the config.

The current solution in the PR adds the .extend parameter to the runtime options that controls the concatArrays parameter of the mergeOptions function in the runtime.

The only solutions, aside from the .extend flag that come to my mind are either concatArrays: true by default (breaking change, also we can't remove transports now, which is like fixing bug A just to create bug B) or the other solution:

By internally chaining functions that take a config and return a config and doing new Libp2p(chainedResult) at the and (where Libp2p is just the vanilla libp2p, not a IPFS-libp2p-runtime) we can simplify this, allow chaining and allow both overriding and extending easily.

This would actually be less complex then my current .extend + chaining implementation since the configuration would now only be a chain of

(libp2pOptions, { datastore, config, ... }) => newOrModifiedLibp2pOptionsObject

Should I implement that instead?

lidel commented 4 years ago

I believe we should not add any additional toggles such as extend, passing libp2p confing is already pretty hard to wrap head around, and we want this API useful outside internal libs.

This sounds better:

By internally chaining functions that take a config and return a config and doing new Libp2p(chainedResult) at the and (where Libp2p is just the vanilla libp2p, not a IPFS-libp2p-runtime) we can simplify this, allow chaining and allow both overriding and extending easily.

We could simplify this furthr and make functions take and return the same "runtime" object as one passed to defaultBundle:

const runtime  = { options, config, datastore, peerInfo, peerBook }

Then, we could create "config pipelines" that apply funcitons like stardust4ipfs as runtime-processors, and also support overriding of Libp2p class initialized at the end of it:

{
  libp2p: [
    (runtime) => stardust4ipfs(runtime), // returns updated runtime2 (passed to the next one)
    (runtime2) => brave4ipfs(runtime2),    // returns updated runtime3
    (runtime3) => new CustomLibp2p(runtime3.options) // optional (implicit call with regular Libp2p if omitted from pipeline)
  ]
}

If subclassing of Libp2p is not needed, short notation would be pretty neat:

{
  libp2p: [
    require('stardust4ipfs')
    require('brave4ipfs')
  ]
}

mkg20001 commented 4 years ago

The problem here is still, that the runtime is mixing in transports on it's own.

As long as we use CustomLibp2p, basically, we'll have to deal with it overriding them

What if, instead, we mix in the transports in the config pipelines. So we'd have something like this:

{
  libp2p: [ // all are functions (runtime) => newRuntime
    IPFS.libp2p.defaultOptions, // pubsub, delegates (defaultBundle)
    IPFS.libp2p.platformOptions, // transports (TCP for node, WS for browser, etc),
    require('stardust4ipfs'),
    require('brave4ipfs')
  ] // the resulting runtime.options will be passed to vanilla `Libp2p` class
}

This would save us the "runtime-layer" which is causing the trouble with overriding the transports array. (The IPFS.libp2p.* functions could also be added at the beginning automatically)

(The IPFS.libp2p.* variables are just exposing the functions for convinience, so users don't have to require('ipfs/src/core/...'))

lidel commented 4 years ago

@mkg20001 Hot sure if this is helpful, but I took API proposed by @alanshaw in https://github.com/ipfs/js-ipfs/pull/2591 and used it for customizing transports of JS-IPFS in Brave – see my PR draft at https://github.com/ipfs-shipyard/ipfs-companion/pull/811.

It is bit simpler, instead of globals you change libp2pOptions: in mentioned PR I customize libp2pOptions.modules.transport and libp2pOptions.modules.peerDiscovery, effectively overriding runtime defaults.

Feels "good enough".

ipfs / js-ipfs