feat: improved error handling on the CLI #1335

hugomrdias · 2018-05-02T14:38:31Z

This PR should improve error handling and help messages in the cli. All messages printed to the console follow the go implementation.

In order to do this i used the yargs fail() method to catch and handle errors from the commands (https://github.com/ipfs/js-ipfs/blob/feat/better-errors-cli/src/cli/bin.js#L81-L104).
Previously we were using the parse() method which isn't made to handle errors, its for running yargs in headless mode to build cli tests.

The fail() method handles promises returned from the commands handler, so its the perfect top-level spot to catch errors and output help specific to the cli. Like this:
https://github.com/ipfs/js-ipfs/blob/feat/better-errors-cli/src/cli/bin.js#L87-L90
Another nice thing is that we can hide the stack trace behind a flag --debug or -D.

In the PR i keep an hybrid solution only daemon, init and id commands are ported to return promises and use fail(). Everything else works almost the same, only two things changed:

moved away from yargs singleton mode, now we actually have two different instances here and here
all errors are actually outputted correctly

The ultimate goal if everyone agrees would be to port all commands to something like the 'id' command included in the PR and start creating errors with codes like this

Object.assign(new Error('repo is not initialized yet'),
{
    code: 'ERR_REPO_NOT_INITIALIZED',
    path: node._repo.path
})

Basically just adding error codes just like node does, so we can do this

if (err.code === 'ERR_REPO_NOT_INITIALIZED') {
     return `No IPFS repo found in ${err.path}. 
please run: 'ipfs init'`
}

instead of this

if (err.message.match(/not found/) || // indexeddb
    err.message.match(/ENOENT/) || // fs
    err.message.match(/No value/) // memory
) {
    return cb(null, false)
}

In the end we could delete all this and do all the cli specific error handling inside fail().

Needs:

chore: update ipfs-repo js-ipfsd-ctl#241

Fixes #1180

daviddias

dismiss my review. wrong button

daviddias · 2018-05-06T16:15:04Z

Needed PRs have been reviewed and merged. Not blocked anymore

hugomrdias · 2018-05-14T14:37:15Z

@diasdavid this PR is complete and with ipfs/js-ipfsd-ctl#241 linked tests pass locally.

daviddias · 2018-05-14T18:56:56Z

@alanshaw mind doing the last review?

daviddias · 2018-05-14T19:38:35Z

I'm puzzled on why the "better CLI errors" is the fix for changing the error on ipfs-repo introduced in -- ipfs/js-ipfs-repo@755b5c6 --.

alanshaw · 2018-05-14T20:55:44Z

src/cli/bin.js

+const { disablePrinting, print, getNodeOrAPI } = require('./utils')
+const addCmd = require('./commands/files/add')
+const catCmd = require('./commands/files/cat')
+const getCmd = require('./commands/files/get')


It's faster if these 3 are only required when needed

fixed with last commit

alanshaw · 2018-05-14T20:58:02Z

src/cli/bin.js

+ipfs - Global p2p merkle-dag filesystem.
+
+  ipfs [options] <command> ...`
+  const MSG_EPILOGUE = `Use 'ipfs <command> --help' to learn more about each command.


Minor - too much indentation!

Sorry where? 26 has no identation. It's exactly the same as the go impl.

i see now, its const eslint auto fixes that wrong.

fixed with last commit

alanshaw · 2018-05-14T21:39:25Z

src/core/index.js

+ * @param {object} stateOptions - Node state options to reject with error
+ * @returns {IPFS}
+ */
+IPFS.createNodePromise = (options = {}, stateOptions = {}) => {


The name suggests that the only difference between it and createNode is that it returns a promise, but actually it creates a node and waits for it to be ready. What about IPFS.startNode()?

Secondly, since you're adding a new public API function here, is it better to offer a dual promise/callback function like the rest of the API? I'd promisify this.

Oh, if this is going to add a new public API, it also needs to add documentation for it in the README: https://github.com/ipfs/js-ipfs/blob/master/README.md#api

This is a static factory method, with the objective of creating a node if it is started or initialised depends on the options.
A startNode factory would imply options = {start:true}.
I can just move this to utils.js and the scope would be cli only.

alanshaw · 2018-05-14T21:44:03Z

src/cli/commands/id.js

+      .then(node => Promise.all([Promise.resolve(node), node.id()]))
+      .then(([node, id]) => {
+        print(JSON.stringify(id, '', 2))
+        node.stop()


How come stop() is called here but not in version.js?

Do all our commands have to remember to call stop() after they're done, and also catch any error and call stop() to clean up correctly? Would it be better to have IPFS created and destroyed once in bin.js like it is at the moment? Is there another benefit to moving the IPFS node getter into each command?

I'm not sure why this command is requiring a online node? Shouldn't this just work and return the ID without having to start a node? Feels wildly inefficient.

Looking at what go-ipfs does: when you call ID with a online daemon, it returns the ID object but with swarm address. When offline, it returns the same but with addresses empty.

In contrast, what this command seems to be doing is to either grab the API if daemon is running but if none is running, start a node.

That too, but I think that should be fixed outside of this PR.

The more i look, more code paths i see, events, cb(err), fsm, etc. This is tricky. I'm gonna push couple commits to improve further on this.
But still to answer your questions:

How come stop() is called here but not in version.js?

you r right every cmd needs to do cleanup (easier cleanup coming in the next commits)

Do all our commands have to remember to call stop() after they're done, and also catch any error and call stop() to clean up correctly? Would it be better to have IPFS created and destroyed once in bin.js like it is at the moment? Is there another benefit to moving the IPFS node getter into each command?

I think being explicit and each cmd control their own lifecycle is better. Also we could remove all this code https://github.com/ipfs/js-ipfs/blob/master/src/cli/bin.js#L73-L104, which got even bigger in this PR after adding more safe guards and error handling. In the end we would only have one code path ending in fail() and proper stack traces, as we do now for daemon, init, id and version.

I'm not sure why this command is requiring a online node? Shouldn't this just work and return the ID without having to start a node? Feels wildly inefficient.
Looking at what go-ipfs does: when you call ID with a online daemon, it returns the ID object but with swarm address. When offline, it returns the same but with addresses empty.
In contrast, what this command seems to be doing is to either grab the API if daemon is running but if none is running, start a node.

It works exactly like that now

{ "id": "QmYHif2UK4aFaMYBiNZYrA7xLvYJajYSQt14H5tmHyagvV", "publicKey": "CAASpgIwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDBJRc5+YxkfPX1LfwUXGY18ezEUpsUmWLAQxReOdeedbEHly8O7WYfYW1iDV1obVttDAbsYLMLh2gmoPqyMjvcAtA/JFnkCjTJ1GCAJFPvrHly5ZFO67knMTHXfVTaKJRBs98JnwrVgFRAbZAwoeVBRbPeP53FOmiw3pXoraOXU7YvGV3vc29PUTn4+1g4seoznyHAx5Ezp52MspiBvb0QmkEGvqZEKFRBUSz8pqMPCDYtItXMzOoBJG29TxxUztGTHvfogdxY9EOpPxVW6Nm0YRWbs9Vc8m/Vjtb88VQGrXXiplgag8OzyT8qM7Axx44nqOkAnQM3L50lOJ/ezGr9AgMBAAE=", "addresses": [], "agentVersion": "js-ipfs/0.28.2", "protocolVersion": "9000" }

alanshaw · 2018-05-14T22:05:07Z

src/cli/commands/init.js

      repo: new Repo(path),
      init: false,
      start: false
-    })


Does createNodePromise resolve when start: false?

yes, ready is always emitted if an error doesn't happen
https://github.com/ipfs/js-ipfs/blob/feat/better-errors-cli/src/core/boot.js#L53

alanshaw · 2018-05-14T22:05:42Z

src/cli/utils.js

  }

-  // Required inline to reduce startup time


Please leave this comment

fixed with last commit

alanshaw · 2018-05-14T22:06:13Z

src/cli/utils.js

  const IPFS = require('../core')
-  const node = new IPFS({
+  return IPFS.createNodePromise({
    repo: exports.getRepoPath(),
    init: false,
    start: false,


Also here, does createNodePromise resolve when start is false?

yes, ready is always emitted if an error doesn't happen
https://github.com/ipfs/js-ipfs/blob/feat/better-errors-cli/src/core/boot.js#L53

fix indentation in bin.js add back inline require comment in utils.js

…eRepoInitialized forceRepoInitialized represents better to objective of the option forceInitialized could confuse we node initiliazed instead of repo

boot always runs first and puts state in stopped state, this check doesnt make sense self.state.init() does nothing fsm even prints an error, check with DEBUG=* js-ipfs init

victorb · 2018-07-10T07:12:28Z

This is marked as "blocked" but it's unclear why it's blocked.

victorb · 2018-08-06T13:55:00Z

What's the progress here or why is it blocked?

Being able to tell when a CLI command have finished running enables us to implement ipfs-exec for the CLI tests differently, where we can massively speed up the nodejs tests for js-ipfs.

alanshaw · 2019-01-23T17:14:07Z

@hugomrdias I think #1834 ended capturing a lot (if not all) of what this PR was trying to address. Shall we close this PR (it's super out of date now) or do you think there are parts of this PR that would still be useful and that aren't already address or about to be addressed by near future PRs (I'm thinking async/await endeavour)?

hugomrdias · 2019-01-28T11:26:09Z

i still need this PR, ill close it when i finish the error codes PR

daviddias · 2019-10-11T15:11:55Z

Hi! js-ipfs master just got a whole new set of automated tests with #2528, #2440 and also running some of the test suites from our early testers (hi5 to @achingbrain for setting it all up!). Would you mind rebasing the master branch on this PR to ensure it runs all the latest tests? Thank you!

ghost assigned hugomrdias May 2, 2018

ghost added the status/in-progress In progress label May 2, 2018

hugomrdias added the status/blocked Unable to be worked further until needs are met label May 2, 2018

hugomrdias requested a review from daviddias May 2, 2018 16:03

ghost assigned daviddias May 4, 2018

daviddias approved these changes May 4, 2018

View reviewed changes

daviddias suggested changes May 4, 2018

View reviewed changes

daviddias removed the status/blocked Unable to be worked further until needs are met label May 6, 2018

daviddias removed their assignment May 6, 2018

daviddias mentioned this pull request May 8, 2018

⚡️ v0.29.0 RELEASE 🚀 #1320

Closed

30 tasks

This was referenced May 8, 2018

Spawn callback can be called multiple times ipfs/js-ipfsd-ctl#239

Closed

Issue introduced on 0.20.1 ipfs/js-ipfs-repo#165

Closed

chore: update ipfs-repo ipfs/js-ipfsd-ctl#241

Closed

daviddias requested a review from alanshaw May 8, 2018 17:58

hugomrdias force-pushed the feat/better-errors-cli branch from 48b07e4 to 49571a2 Compare May 8, 2018 18:21

hugomrdias added the status/blocked Unable to be worked further until needs are met label May 9, 2018

hugomrdias added 5 commits May 14, 2018 14:03

chore: add vscode and eslint to gitignore

43c0c13

feat(cli): improve error handling and help messages

6924e35

test(cli): fix tests for new error messages

40111af

fix: fix version cmd and repo component

6dbb834

fix: resolve conflits with the new inline requires

c649a98

hugomrdias force-pushed the feat/better-errors-cli branch from 49571a2 to c649a98 Compare May 14, 2018 14:36

hugomrdias changed the title ~~WIP: Feat/better errors cli~~ Feat/better errors cli May 14, 2018

chore: remove only from http-api tests

2bdfeee

alanshaw suggested changes May 14, 2018

View reviewed changes

fix: add back inline require in bin.js and some small stuff

43bcafa

fix indentation in bin.js add back inline require comment in utils.js

hugomrdias added 3 commits May 15, 2018 16:27

fix: remove not needed Promise.resolve and change stateOption to forc…

34bab95

…eRepoInitialized forceRepoInitialized represents better to objective of the option forceInitialized could confuse we node initiliazed instead of repo

feat: add clean method to make node cleanup easier

7209da3

fix: remove unneeded check in init

7f6fccc

boot always runs first and puts state in stopped state, this check doesnt make sense self.state.init() does nothing fsm even prints an error, check with DEBUG=* js-ipfs init

fsdiogo mentioned this pull request May 16, 2018

Better error handling in daemon + gateway #1325

Closed

daviddias changed the title ~~Feat/better errors cli~~ feat: improved error handling on the CLI May 29, 2018

hugomrdias mentioned this pull request Sep 24, 2018

2018 Q4 OKRs Planning #1566

Merged

hugomrdias closed this Feb 4, 2020

hugomrdias deleted the feat/better-errors-cli branch February 4, 2020 14:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improved error handling on the CLI #1335

feat: improved error handling on the CLI #1335

hugomrdias commented May 2, 2018 •

edited

Loading

daviddias left a comment

daviddias commented May 6, 2018

hugomrdias commented May 14, 2018

daviddias commented May 14, 2018

daviddias commented May 14, 2018

alanshaw May 14, 2018

hugomrdias May 15, 2018

alanshaw May 14, 2018

hugomrdias May 15, 2018

hugomrdias May 15, 2018

hugomrdias May 15, 2018

alanshaw May 14, 2018 •

edited

Loading

Mr0grog May 14, 2018

alanshaw May 15, 2018

hugomrdias May 15, 2018

alanshaw May 14, 2018

victorb May 14, 2018

alanshaw May 15, 2018

hugomrdias May 15, 2018

alanshaw May 14, 2018

hugomrdias May 15, 2018

alanshaw May 14, 2018

hugomrdias May 15, 2018

alanshaw May 14, 2018

hugomrdias May 15, 2018

victorb commented Jul 10, 2018

victorb commented Aug 6, 2018

alanshaw commented Jan 23, 2019

hugomrdias commented Jan 28, 2019

daviddias commented Oct 11, 2019

feat: improved error handling on the CLI #1335

feat: improved error handling on the CLI #1335

Conversation

hugomrdias commented May 2, 2018 • edited Loading

daviddias left a comment

Choose a reason for hiding this comment

daviddias commented May 6, 2018

hugomrdias commented May 14, 2018

daviddias commented May 14, 2018

daviddias commented May 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanshaw May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorb commented Jul 10, 2018

victorb commented Aug 6, 2018

alanshaw commented Jan 23, 2019

hugomrdias commented Jan 28, 2019

daviddias commented Oct 11, 2019

hugomrdias commented May 2, 2018 •

edited

Loading

alanshaw May 14, 2018 •

edited

Loading