Message set in custom constructor gets overwritten

[apaleslimghost]

The custom constructor is called before the message is saved from the arguments, so any message set in the constructor is overwritten.

[emmenko]

Up

[mgmorcos]

+1

[wspringer]

+1

[diegovdev]

+1

[dcollinsf5]

+1
Although, looking at the source code, I was able to make this do what I wanted; it seems not obvious from the documentation, but any strings passed in to your custom constructor are placed into an array that becomes the 'message' property of your error, and any errors passed in to your custom constructor are used to build a joined stack trace from all errors including the current one.

So, your custom constructor need only look at the first argument; your 'properties' object. the rest of the arguments supplied should be errors or message strings.

My first attempt was:

function throwDestroyable(message, origErr) {
  throw new DestroyableError({
    message:      message + ': ' + origErr.message,
    user:         this.taskContext.user,
    taskParams:   this.taskContext.taskParams
  }, origErr);
},

Which, as we know, results in message being overwritten. But instead, I use:

function throwDestroyable(message, origErr) {
  message += ': ';
  throw new DestroyableError({
    user:         this.taskContext.user,
    taskParams:   this.taskContext.taskParams
  }, origErr, message, origErr.message);
}

Which produces a combined stack for both my original error and the new error I am creating as well as a 'message' property which is an array of both the passed in message argument and the original error's message. Try it :)

[jproulx]

What would be the preferred action here? The original intent was to allow a default message to be set when no message was passed, but always used the passed in message to override the "default".

[dcollinsf5]

I feel like the current behavior of walking arguments and building 'messages' from all strings and numbers (why numbers in 'messages'?) is fine for any custom error generator without a custom constructor, but if you use a custom constructor, things are very tricky in the current implementation; it seems like the code tries to maintain non custom constructor benefits but doesn't differentiate at all between arguments that the custom constructor will be using and arguments that the internal code will be using; the developer has to read the source to figure out that arguments meant for their custom constructor will end up in 'messages' if they are strings or numbers, for example.

I feel that the main problem is actually with a documentation issue on the main page, which clearly indicates that you should be able to override this.message in your custom constructor, even though that is not possible. Incidentally, and more confusingly to new users of the toolkit, your example will appear to kind of work, since the code will build 'message' from both arguments provided (a string and a number, per here:

node-custom-error/error.js

Line 80 in 5805fc1

} else if (typeof param == 'string' || typeof param == 'number') {

), resulting in a message of 'You do not have permission 403', whereas your example would indicate you should see: 'You do not have permission' all by itself.

I see two options:

(A) Update the documentation and leave it as it is now. People would need to be aware that custom constructors cannot change .message, and more importantly, that any strings or numbers passed into their custom constructors as arguments will become the content of the errors 'message' value. Also, it should be stressed that when using a custom constructor, the code uses the first argument as the 'message' parameter for creating the base Error internally.

As in my example in my post from earlier today above, I can work with this by (a) writing my custom constructors to only use/pay attention to the first argument I pass in to the constructor, which is a configuration object the constructor uses internally, (b) passing in previous errors that I was the stack from as arguments, and (c) passing in strings (or numbers, though i don't quite understand why numbers should be 'message'...) that I want to become the 'message' property as other arguments to the function.

In this case, the code would still create a new Error internally(incorrectly?) (

node-custom-error/error.js

Line 61 in 5805fc1

var proxy = new Error(message);

) with my 'configuration object' passed as the 'message' to the Error constructor, but the loop on rows 76-83 would still be used to automatically combine error object stacks, and for building a message with multiple strings without any boilerplate code in my custom constructor. It's kind of awkward, since it requires those message strings be built before I make the call to create a custom error, and it'd be nice for encapsulation if the error's custom constructor could do that for me. At the very least, documentation needs to indicate that even with a custom constructor, all arguments passed into that constructor are still processed as if there was no custom constructor internally to avoid confusion like we have now. At the very least, it would be really nice if the same check you do for errors on line 81 was implemented for messages on line 93; if the messages array is completely empty, it should not overwrite .message with an empty string (which it does now) Still, anyone who wants to pass strings into their custom constructor and set a message will have problems; it doesn't really address the core issue.

Or, alternatively, (B) - you can go for an API change to fix this stuff up, while still allowing users to pick-and-choose which parts of your automatic error merging and message building logic that they want to leverage. If it was me, that's what I'd do; a full version # bump, as it would break backwards compatibility, and it would work something like:

Have two modes for the error generator - default mode would work exactly as it does now, and would be used any time there is no custom constructor defined on the error generator, but in if a custom constructor is defined, then 'custom mode' would run, where arguments passed in would be identifiable as to their purpose, either positionally or by their key in a passed in object hash.

For example, where non-custom-constructor assumes first argument is the message string, and all further arguments are either message elements (if a number or string) or error objects to use for their stack properties, custom-constructor mode would inspect the passed arguments and identify if it needed to do any message or error stack processing based on them.

Positionally:

var myError = customConstructorError(
                 customConstructParams{},
                 messages[],
                 errorObjects[])

or with an object hash with identifiable keys:

var myError = customConstructorError({
                   customConstructParams: {...},
                   messages: (String || String[...]),
                   errors: (Error || Error[...]
                 ))

I tend to prefer the object hash method.

Only the 'customConstructParams' key of the passed object (or the first argument) would be provided to the custom constructor; and the other two keys/arguments would specifically be for internal use.

If the developer wanted to still get stack aggregation from errors, but set their very own message logic internally in their constructor method, then they would call:

var myError = customConstructorError({
                   customConstructParams: { /* passed to custom constructor */ },
                   errors: (prevError || [prevError1, prevError2,...])
                 })

In this case, no logic would fire to manage this.message, because no 'messages' property was passed in, so custom messages logic would be performed in the custom contructor and not effected by internal logic.

If, on the other handle, someone wanted to have messages set for them simply by passing them in as strings, but still have custom constructor logic for other tasks, they could call:

var myError = customConstructorError({
                   customConstructParams: { /* passed to custom constructor */ },
                   errors: prevError,
                   messages: ["Message1:", "message2!!!"]
                 })

You could even check when building 'messages' to be sure that the developer had not modified this.messages in their custom constructor, and show a warning if they did, with instructions on how to resolve the problem. If the developer wanted complete control over stack aggregation AND message creation (and anything else), they could pass errors and messages in as keys to customConstructParams, and do all of their own logic in their custom constructor; since they pass no 'messages' or 'errors' keys to the customErrorGenerator, it would helpfully step aside and let them do whatever they want. Right now, if you wanted to pass an Error object into your custom constructor, you would also be accepting that the Error would effect your stack because of how the arguments are walked.... unless you do something hacky, like wrap it in an object... not too flexible, and hard to reason about how arguments you are passing in will effect your end result error object.

Just brainstorming on a Friday, there could be problems with this approach I haven't thought of :)

[FunkMonkey]

Just stumbled upon this problem. Any updates? Are pull requests for this welcome?