TritonDataCenter / node-verror

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

VError.info doesn't return custom properties on non-VError cause children #79

Open jjm340 opened 4 years ago

jjm340 commented 4 years ago

I have an error class that adds custom properties to itself:

export class ValidationError extends Error {
  public responseCode = ResponseCode.ValidationError;
  constructor(message: string, public data: any) {
    super(message);
  }
}

I have a logger method thats supposed to grab all the keys and scrub them from the error chain:

exception: (message: string, ex: Error, data?: Record<string, unknown>) => {
    const errorToLog = new VError(
      {
        cause: ex,
        info: data,
      },
      message,
    );

    const info = VError.info(errorToLog);
    console.log('info: ', info);

    logError(message, errorToLog);
  }

But when I pass an instance of the first error as the cause to VError, the VError.info method doesn't enumerate the keys for the ValidationError. Is this by design?

davepacheco commented 4 years ago

If I'm understanding right, that's the way it's intended. This bit in the README explains why we did it this way:

In order for all this to work, programmers need to know that it's generally safe to wrap lower-level Errors with higher-level ones. If you have existing code that handles Errors produced by a library, you should be able to wrap those Errors with a VError to add information without breaking the error handling code. There are two obvious ways that this could break such consumers... The error's properties might change. People often hang additional properties off of Error objects. If we wrap an existing Error in a new Error, those properties would be lost unless we copied them. But there are a variety of both standard and non-standard Error properties that should not be copied in this way: most obviously name, message, and stack, but also fileName, lineNumber, and a few others. Plus, it's useful for some Error subclasses to have their own private properties -- and there'd be no way to know whether these should be copied. For these reasons, VError first-classes these information properties. You have to provide them in the constructor, you can only fetch them with the info() function, and VError takes care of making sure properties from causes wind up in the info() output.

Sorry if I'm misunderstanding!

jjm340 commented 4 years ago

For these reasons, VError first-classes these information properties. You have to provide them in the constructor, you can only fetch them with the info() function, and VError takes care of making sure properties from causes wind up in the info() output.

This part of the documentation makes it sound like it should surface these custom properties.

jjm340 commented 4 years ago

Also what about other libraries that I have no control over?

davepacheco commented 4 years ago

When it says "properties from causes", it's referring to the same first-class informational properties that were provided in the VError constructor on those causes. Right now, you'd have to wrap errors from other libraries if you wanted to preserve this information. It's possible there's a better approach here? But the rest of the text explains our constraints on the problem and why it seemed fraught to just copy all the properties that we find.

jjm340 commented 4 years ago

Okay, so I wrap other errors in VError to get those moved up. Got it. Makes sense