884342 - change the app.use error handler to return the correct codes

Status: RESOLVED FIXED

(Webmaker Graveyard :: Thimble, defect)

Description

[Pomax]

we should replace all "new Error" errors with soft errors that don't crash the node process. Or change the code so that Error objects don't crash node.

Comment 1

[Jon Buckley [:jbuck]]

They shouldn't cause node to crash, just have Express dump an error

Comment 2

[Pomax]

this is caused by the error handling in app.js:

app.use( function( err, req, res, next) {
  res.send( 500, err );
});

we need to switch that over to something that checks what the error actually is, and responds appropriately =)

Comment 3

[Pomax]

See https://github.com/mozilla/popcorn.webmaker.org/blob/master/lib/utilities.js#L34-L38 and https://github.com/mozilla/popcorn.webmaker.org/blob/master/server.js#L139-L146

Comment 4

[Pomax]

unassigning pending reprioritisation

Comment 5

[Elsi [:elnushaj]]

I'm willing to give this a go.

Comment 6

[Elsi [:elnushaj]]

Could I get a little more detail on what is required for this bug, I.E. what errors I should be checking for and possibly what kind of responses (or the format of the responses) that should result.

Comment 7

[Elsi [:elnushaj]]

Attachment: https://github.com/mozilla/thimble.webmaker.org/pull/285

Comment 8

[Pomax]

Comment on attachment 831021 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/285

r+ if you remove the superfluous comma

Comment 9

[Pomax]

landed