Open felixfbecker opened 1 year ago
removing stack logging can be troublesome, although with promises it anyways has become troublesome since promises just swallow all the information. I can't possibly merge this, as you're breaking the functionality by removing the explicit logging of the stack. Maybe you can adjust it so the stack logging still happens, or at least can be configured.
I'm not sure I understand – isn't the stack still logged in the screenshot above? The information logged now is a strict superset of what was logged before
The stack does not get logged by default on all error objects. That is why.
Are you sure? What's an example of an error object that wouldn't have the stack logged by console.log()
?
For reference, here's the code branch in NodeJS that formats errors:
isError()
returns true
for any Error
instance, even from other realms (see https://nodejs.org/api/util.html#utiltypesisnativeerrorvalue).
If for some reason a non-Error instance was to be thrown with a stack
property, it would still get logged as a property of a plain object, just not formatted quite as pretty. I think that is an okay trade-off because that's kind of what you have to expect when throwing a non-Error instance, it's not going to get formatted as an Error, but as an object with properties and values. It would actually still print information than currently, because currently no other properties on such object except stack
get logged.
Note also that the current code already doesn't support this case well, for example if somebody was to throw null
the current code would throw because it's trying to read stack
of null
. The code in this PR would actually handle this more gracefully.
But if you'd like I could add an extra if
that checks for the case of a non-Error
object with a stack
property and in that case log the stack
instead.
I second what @felixfbecker said, if the stack isn't being logged that's most likely an issue with the underlying logging framework, explicitly logging the error but not the stack is definitely not the norm and it removes a lot of useful information.
Currently, when using SQL mode and a migration contains a syntax error,
db-migrate
will only print the error message and call stack:Even for a medium sized migration SQL file,
syntax error at or near ")"
can be like finding a needle in a haystack.When using
pg
, the Error actually contains additional information with the exact position of the syntax error. But currently, that information is not logged, as explicitly only thestack
of the error is logged.This PR changes it so that the entire error object is logged. Node's behavior when an
Error
is passed toconsole.error()
(which is used under the hood bylog.js
) is to log only thestack
property if there are no additional properties, meaning there would be no difference in those cases.But if there are additional properties, Node will log both the
stack
with message, and any properties after it:In particular, the
position
here can be copy-pasted into an editor's "go to position" command to jump exactly to the syntax error in the SQL file. Other properties can be helpful in case e.g. a DB constraints gets violated by the migration, as it can contain the constraint name, offending column values, etc.One small change is also necessary to make sure the error gets properly logged: Throw the actual error if it's an
Error
instance instead of wrapping it in anAssertionError
, as the AssertionError just adds additional noise to the log and since the Error gets nested under theactual
property, it doesn't get expanded in the log but reduced to only[DatabaseError]
. There may be a small benefit in wrapping in an error given the error may not be anError
instance, but if it is, it makes more sense to throw the error directly.