TritonDataCenter / node-verror

Rich JavaScript errors
MIT License
1.18k stars 61 forks source link

[Question] Why prepend message with a colon? #78

Open blackdynamo opened 4 years ago

blackdynamo commented 4 years ago

Is there a reason why you prepend : before appending the cause message? I might be missing something deeper in the library that requires that additional colon. I have noticed it when trying to use the VError library and it's adding the "unexpected" :.

https://github.com/joyent/node-verror/blob/master/lib/verror.js#L185

    if (cause) {
        mod_assertplus.ok(mod_isError(cause), 'cause is not an Error');
        this.jse_cause = cause;

        if (!parsed.options.skipCauseMessage) {
            message += ': ' + cause.message;
        }
    }

The following code:

import {VError} from "verror";

const err = new VError(new Error("Original"));
console.log(err);

Outputs the following:

VError: : Original
    at Object.<anonymous> (example.js:3:13)
    at Module._compile (internal/modules/cjs/loader.js:955:30)
    at Module._compile (/node_modules/pirates/lib/index.js:99:24)
    at Module._extensions..js (internal/modules/cjs/loader.js:991:10)
    at Object.newLoader [as .js] (/node_modules/pirates/lib/index.js:104:7)
    at Module.load (internal/modules/cjs/loader.js:811:32)
    at Function.Module._load (internal/modules/cjs/loader.js:723:14)
    at Function.Module.runMain (internal/modules/cjs/loader.js:1043:10)
    at Object.<anonymous> (/node_modules/@babel/node/lib/_babel-node.js:174:21)
    at Module._compile (internal/modules/cjs/loader.js:955:30) {
  jse_shortmsg: '',
  jse_cause: Error: Original
      at Object.<anonymous> (example.js:3:24)
      at Module._compile (internal/modules/cjs/loader.js:955:30)
      at Module._compile (/node_modules/pirates/lib/index.js:99:24)
      at Module._extensions..js (internal/modules/cjs/loader.js:991:10)
      at Object.newLoader [as .js] (/node_modules/pirates/lib/index.js:104:7)
      at Module.load (internal/modules/cjs/loader.js:811:32)
      at Function.Module._load (internal/modules/cjs/loader.js:723:14)
      at Function.Module.runMain (internal/modules/cjs/loader.js:1043:10)
      at Object.<anonymous> (/node_modules/@babel/node/lib/_babel-node.js:174:21)
      at Module._compile (internal/modules/cjs/loader.js:955:30),
  jse_info: {},
  message: ': Original'
}

Please note the error.message is ": Original" with the colon.

jclulow commented 4 years ago

I suspect this is an oversight. It was generally envisaged that you would add extra context when wrapping your error; e.g.,

var err = new VError(new Error("Original"), 'thing I was doing');

Which would then print out, e.g.,

VError: thing I was doing: Original

I think you could make the case that constructing a VError with a cause, but no message, could just as well not insert the colon.

blackdynamo commented 4 years ago

Interesting. Thanks for the prompt response. I am not exactly sure in my case what I would add as a "context", so for me, it would likely be beneficial to only add the : if a context aka sprintf_args are provided.

If this is something the maintainers are comfortable with, I can look into putting a PR together.

davepacheco commented 4 years ago

Yes, as @jclulow mentioned, a primary use-case for VError is to add context to an existing error. See the last example in this section for how the messages are expected to compose together (with the colon): https://github.com/joyent/node-verror#quick-start

Out of curiosity, why would you wrap an existing error in a VError with no additional context? Is that because the Error is coming from someplace else that doesn't use VError and you want to use some other VError features on it?

blackdynamo commented 4 years ago

Hey Dave,

We are using the VError both as a base error and to wrap expected errors with metadata aka info. With the way async errors work with the stack it's nice to use the cause to maintain the stack from the original error.

Right now I would do something like this:

class ValidationError extends VError {}
class DomainValidationError extends VError {}

So for example when using a schema validation library that throws a standard error, we can capture that error, wrapped in a ValidationError. It will have the original cause and we can tack on some additional information as to what schema was used, etc. Somewhere else in the system can catch errors and log properly the ValidationError. That same catch will re-throw anything that' isn't a ValidationError.

       DomainValidationError: : 'payload.timestamp' should be string
           at ../src/combine-domain-validators.js:22:13
           at runMicrotasks (<anonymous>)
           at processTicksAndRejections (internal/process/task_queues.js:94:5)
       caused by: ValidationError: 'payload.timestamp' should be string
           at ../src/validate-json.js:41:11
           at validator (../src/create-json-validator.js:16:11)
           at validate (../src/combine-domain-validators.js:20:13)
           at execute (../../src/application/application.js:47:13)
           at execute (../src/handle-message.js:31:14)
           at ../src/handle-message.js:49:18
       info: {
         "descriptor": "system.incoming-message.timeSchema"
       }

We could definitely write our own base Error class but VError seems to have all the bonus functionality that we would want, with the exception of the :.

bertho-zero commented 4 years ago

The https://www.npmjs.com/package/@openagenda/verror version fix that.

err = new VError(new Error()); // VError
console.log(err);

// ...

err = new VError(new VError(new VError(new VError('Test')), 'Ok')); // VError: Ok: Test
expect(err.message).toEqual('Ok: Test');

gives me

VError
    at Object.<anonymous> (/home/bertho/oa/packages/verror/lib/verror.js:349:11)
    at Module._compile (internal/modules/cjs/loader.js:1158:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1178:10)
    at Module.load (internal/modules/cjs/loader.js:1002:32)
    at Function.Module._load (internal/modules/cjs/loader.js:901:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:74:12)
    at internal/main/run_main_module.js:18:47 {
  jse_shortmsg: '',
  jse_cause: Error
      at Object.<anonymous> (/home/bertho/oa/packages/verror/lib/verror.js:349:22)
      at Module._compile (internal/modules/cjs/loader.js:1158:30)
      at Object.Module._extensions..js (internal/modules/cjs/loader.js:1178:10)
      at Module.load (internal/modules/cjs/loader.js:1002:32)
      at Function.Module._load (internal/modules/cjs/loader.js:901:14)
      at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:74:12)
      at internal/main/run_main_module.js:18:47,
  jse_info: {}
}
ncjones commented 3 years ago

We generally do not provide a custom message when constructing a VError as it adds little value compared to specifying name and info such as:

catch (err) {
  throw VError({
    name: 'UpdateGizmoFailed',
    cause: e,
    info: { gizmo },
  });
}

Having said that it would still be nicer if the message property had a more sensible default. For the above usage it would make sense to use the error name concatenated with the cause's message.

bertho-zero commented 3 years ago

@ncjones with @openagenda/verror if I have:

const VError = require('@openagenda/verror');

try {
  throw new Error('nimp');
} catch (err) {
  throw new VError({
    name: 'UpdateGizmoFailed',
    cause: err,
    info: { gizmo: 'your-obj' },
  });
}

it throws:

/home/bertho/dev/verror/truc.js:6
  throw new VError({
  ^
UpdateGizmoFailed: nimp
    at Object.<anonymous> (/home/bertho/dev/verror/truc.js:6:9)
    at Module._compile (node:internal/modules/cjs/loader:1092:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1121:10)
    at Module.load (node:internal/modules/cjs/loader:972:32)
    at Function.Module._load (node:internal/modules/cjs/loader:813:14)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:12)
    at node:internal/main/run_main_module:17:47 {
  shortMessage: '',
  cause: Error: nimp
      at Object.<anonymous> (/home/bertho/dev/verror/truc.js:4:9)
      at Module._compile (node:internal/modules/cjs/loader:1092:14)
      at Object.Module._extensions..js (node:internal/modules/cjs/loader:1121:10)
      at Module.load (node:internal/modules/cjs/loader:972:32)
      at Function.Module._load (node:internal/modules/cjs/loader:813:14)
      at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:12)
      at node:internal/main/run_main_module:17:47,
  info: { gizmo: 'your-obj' }
}

I added a meta option which allows for example to add a code property to the error. And also the http errors which uses this meta option.

The following code:

const { VError, BadRequest } = require('@openagenda/verror');

try {
  throw new BadRequest('You doing 💩');
} catch (err) {
  throw new VError({
    name: 'UpdateGizmoFailed',
    cause: err,
    info: { gizmo: 'your-obj' },
    meta: {
      neededHere: 'first level metadata'
    }
  }, 'Gizmo failed to update');
}

will prints:

/home/bertho/dev/verror/truc.js:6
  throw new VError({
  ^
UpdateGizmoFailed: Gizmo failed to update: You doing 💩
    at Object.<anonymous> (/home/bertho/dev/verror/truc.js:6:9)
    at Module._compile (node:internal/modules/cjs/loader:1092:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1121:10)
    at Module.load (node:internal/modules/cjs/loader:972:32)
    at Function.Module._load (node:internal/modules/cjs/loader:813:14)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:12)
    at node:internal/main/run_main_module:17:47 {
  shortMessage: 'Gizmo failed to update',
  cause: BadRequest: You doing 💩
      at Object.<anonymous> (/home/bertho/dev/verror/truc.js:4:9)
      at Module._compile (node:internal/modules/cjs/loader:1092:14)
      at Object.Module._extensions..js (node:internal/modules/cjs/loader:1121:10)
      at Module.load (node:internal/modules/cjs/loader:972:32)
      at Function.Module._load (node:internal/modules/cjs/loader:813:14)
      at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:12)
      at node:internal/main/run_main_module:17:47 {
    shortMessage: 'You doing 💩',
    info: {},
    code: 400,
    className: 'bad-request'
  },
  info: { gizmo: 'your-obj' },
  code: 400,
  className: 'bad-request',
  neededHere: 'first level metadata'
}