expressjs / api-error-handler

Express error handlers for JSON APIs
MIT License
101 stars 23 forks source link

Disable console.error() #3

Closed tkrotoff closed 8 years ago

tkrotoff commented 8 years ago

A call to console.error() is present at line 22.

Would be nice to be able to disable it, especially when writing/running tests (can clutter the terminal output).

blakeembrey commented 8 years ago

@tkrotoff If you're running into error.status >= 500, it's kind of an issue and you should look at fixing it to use better status code (>= 500 are server errors). I don't have any comment on whether there should have a flag to disable it though. In production, you definitely want to be notified of this.

jonathanong commented 8 years ago

i don't mind a flag, but yeah, there's almost no reason these should be logged even in testing since you should be handling them.

tkrotoff commented 8 years ago

As said, I'm not talking about production.

What is your workflow when working on an Express app? As of myself, I do write unit tests for failure cases like:

it('should not create a user without a name', done => {
  request(app).post('/Users/')
    .send({}) // Instead of {name: 'Edgar'}
    .expect(500) // Checks an error occurred
    .end((err, res) => {
      // blabla
    });
});

but then I end up with hundreds of lines like this one in my terminal (*):

error: insert into "Users" default values returning "id" - null value in column "name" violates not-null constraint
    at Connection.parseE (node_modules/pg/lib/connection.js:539:11)
    at Connection.parseMessage (node_modules/pg/lib/connection.js:366:17)
    at Socket.<anonymous> (node_modules/pg/lib/connection.js:105:22)
    at emitOne (events.js:77:13)
    at Socket.emit (events.js:169:7)
    at readableAddChunk (_stream_readable.js:146:16)
    at Socket.Readable.push (_stream_readable.js:110:10)
    at TCP.onread (net.js:523:20)

lines that I don't care about in testing mode: jasmine/mocha already tells me if something is wrong.

(*) that don't occur when console.error() is commented

Edit: error 500 instead of 422

jonathanong commented 8 years ago

well 1, that's a server error so it should be logging that error. your app should be handling the error.

2, it's not a 5xx error, so that that line in this module shouldn't be what's logging the error

tkrotoff commented 8 years ago

@jonathanong

it's not a 5xx error

Pretend you did not see the 422 (I've edited the example) :)

that's a server error [...] your app should be handling the error

I see, my mistake, I should never ever have 5xx errors.

My code lazily pass all PostgreSQL errors to api-error-handler (.query().catch(error => next(error))) without a HTTP status code (so api-error-handler automatically sets it to 500). Instead I should parse PostgreSQL errors and compute the proper HTTP status code.

To follow with the above example: if someone tries to create a user without a name, I should parse the PostgreSQL error and return 422 because of "Integrity Constraint Violation" (23502) instead of letting api-error-handler returning 500.

Hence my terminal cluttered by logs from api-error-handler, sorry for the bad report.