One-liner to create an IPFS node?

[haadcode]

I'd like to propose a function to create an IPFS instance with one line.

Related to #1076, I find myself often using a wrapper to start IPFS with one line:

const ipfs = await startIpfs(ipfsConf)

I've been using a simple wrapper to do that: https://github.com/orbitdb/orbit-db/pull/240/files#diff-55b9c488904893cc93c680eb807f0920.

How we could do this, is to add a static (class) method to the IPFS main class (this:

class IPFS extends EventEmitter {
  constructor (options) {
     ...
  }

  // We could also name the method 'createInstance' or 'from'
  static create (ipfsConfig) {
    return new Promise((resolve, reject) => {
      const ipfs = new IPFS(ipfsConfig)
      ipfs.on('error', reject)
      ipfs.on('ready', () => resolve(ipfs))
      // There may or may not be more here to take into consideration
      // in terms of the IPFS state, please let us know if that's the case.
    })
  }

We would then be able to use it as:

const ipfs = await IPFS.create(ipfsConfig)
// ipfs is ready to use

// Or:
IPFS.create(ipfsConfig)
  .then(ipfs => {
    // ipfs is ready to use
  })

I like the way we create IPFS instance now and wait for the 'ready' event. However, I feel this would be a nice short-hand way to start a node, purely from convenience reasons and to avoid nesting as much as possible. One line! ❤️ 😄

Would be happy to PR this!

What do y'all think?

[Beanow]

Just my two cents, I'm not convinced this is an improvement.

// Current
const ipfs = new IPFS(config)
ipfs.on('error', err => { /* handle error */ })
ipfs.on('ready', () => { /* your logic */ })

// Suggested
IPFS.create(ipfsConfig)
.then(ipfs => { /* your logic */ })

Note that the only line that disappears here is error handling.

// Better, but seems familiar
IPFS.create(ipfsConfig)
.catch(err => { /* handle error */ })
.then(ipfs => { /* your logic */ })

Now we're back to square one. But even that comes at a significant cost.

Error handling in promises isn't great to begin with. But on top of that, promises can't describe the same error handling model that event emitters have with an error event.

In promises, errors need to be associated with the task that caused it.
But with the IPFS node these errors might crop up from something totally unrelated, for example an external node dialing you and sending bad data. Even though you might never have given IPFS a command that led to this external node dialing you.

To put that in other terms. Once your IPFS node is resolved, you no longer have any error handlers in place. It will just silently emit error events into the void.

[pgte]

@Beanow I think we need to be able to associate errors to the given commands.
I think that

await node.start()

is an API that is easy to reason with.

As an app developer that uses IPFS, I think that having an error thrown at you at any given point in time without correlation to the command is not something an app developer should need to handle.

And if that happens, the error is thrown into the void (in the browser) or crashes the node process, and I believe that's the correct behaviour, because that's an IPFS bug, not an app bug, so it's not to be expected by the app.

Related: #1084

[Beanow]

I respectfully disagree, because before making life easier for developers, we should make life easy for the users. In a P2P system where you need to deal with potential rogue nodes trying to attack the ports you've opened, it sounds like a bad idea to settle for a subpar error handling model just to ship apps faster or with a more mainstream syntax.

If anything I think the API should be expanded to have far more detailed, explicit and mandatory error handling. (By mandatory I mean, the entire process should crash when you've not handled the error).

Similar to what you've posted here #1061

[haadcode]

I hear you @Beanow and agree error handling is currently tricky with Promises. Fortunately await and try-catch makes this much nicer imo. I'm looking at this from the perspective of preferring to use await, not necessarily wanting to use Promises.

I agree with @pgte that errors would ideally be associated with a given command and the reasoning from app dev's perspective. I don't think it's about building apps faster but to understand where errors come from and reacting to them as appropriate, per app. Catch-all event handler makes this quite unintuitive imo. That all said, I do agree with @Beanow that it's important that we can understand when things go bad in IPFS and be able to handle it or crash hard.

To rephrase my original examples:

let ipfs
try {
  ipfs = await IPFS.create(ipfsConfig)
} catch (e) {
 // errors during starting IPFS
}
// ipfs is ready to use
// handle errors as appropriate, either via
// ipfs.on('error', ...) or by try-catching specific commands

// Or:
IPFS.create(ipfsConfig)
  .then(ipfs => {
    // ipfs is ready to use
    // handle errors as appropriate, either via
    // ipfs.on('error', ...) or by try-catching specific commands
  })
  .catch(e => /* errors during starting IPFS */)

I also like what @pgte proposes with #1084, it's almost the same what I'm trying to propose here:

try {
  const node = new IPFS(ipfsConfig)
  await node.start()
  try {
    await node.files.get(hash)
  } catch (e) {
    // catch errors specific to files.get()
  }
} catch (e) {
 // all errors
}

[pgte]

@Beanow The IPFS node being an event emitter, users could still listen for emitted errors.
But, IMO, it's very important to make an effort to have correlation between commands and errors.

(Also, I fail to see a distinction between a user and an app developer.
I can see the distinction in the CLI interface, but I fail to see it on the JS API..)

[Beanow]

@pgte absolutely, that would be great. But the suggested sugar only provides the illusion that that is the case. The internals would need to be overhauled and made consistent with that approach. Until then, I say 🙅‍♂️ to an API that gives the wrong impression.

By users I mean the people that are using the applications built on top of js-ipfs. Not a direct consumer of the API. I personally prefer slightly more verbose APIs if that means more resilient applications make it into those user's hands.

[mitra42]

I agree that simplification would be great ... especially in the absence of documentation of configuration options. To that end, I'd suggest that your create() function should include a default Config, something that connects an app to IPFS. (The app developer typically doesnt care how), allow config options to overwrite what is chosen, and have it make the appropriate platform dependent decisions.

[alanshaw]

Lets track this over at #1762 now.

[haadcode]

@alanshaw and others, thank you so much for landing this with the new release! one. line. ❤️ ❤️