Enable express error handler #4697
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4697 +/- ##
==========================================
- Coverage 92.96% 92.91% -0.06%
==========================================
Files 119 119
Lines 8828 8830 +2
==========================================
- Hits 8207 8204 -3
- Misses 621 626 +5
Continue to review full report at Codecov.
|
|
@flovilmart, That option |
|
@flovilmart , is it still missing something? |
| app.use(function (err, req, res, next) { | ||
| next | ||
| lastError = err | ||
| }) |
There was a problem hiding this comment.
Can you follow style and conventions and add the ; at the expected places please?
| const rp = require('request-promise'); | ||
|
|
||
| describe('Enable express error handler', () => { | ||
| beforeEach((done) => { |
There was a problem hiding this comment.
Not sure why you need this, as you create a new server separately. Please remove if not required
There was a problem hiding this comment.
Ok. Really not needed
| const serverUrl = "http://localhost:12667/parse" | ||
| const appId = "anOtherTestApp"; | ||
| const masterKey = "anOtherTestMasterKey"; | ||
| let server; |
There was a problem hiding this comment.
Seems written to, but never read.
There was a problem hiding this comment.
It's used to store the server to be closed later
.then(() => {
server.close(done);
});
| server = app.listen(12667); | ||
|
|
||
| app.use(function (err, req, res, next) { | ||
| next |
There was a problem hiding this comment.
This is just to silence the linter.
The function needs to have 4 arguments to be recognized as an error handler, but I don't want to call next again
| body: { someField: "blablabla"}, | ||
| json: true | ||
| }) | ||
| .then(() => { |
There was a problem hiding this comment.
Can you keep the alignment consistent with the rest of the code? Better, use async / await
There was a problem hiding this comment.
Keeping the alignment :)
| fail('Should throw error'); | ||
| }) | ||
| .catch(e => { | ||
| expect(e).toBeDefined(); |
There was a problem hiding this comment.
Can we do some tests on the expected error here? As well as lastError?
|
Hey @flovilmart , Sorry about the delay. |
|
Not sure why but the build fails on the CI. |
|
@flovilmart , CI fixed now. It was a recent change on |
|
Perfect thanks! |
* Propagate error to express handler in all situations * Call the default error handler if `enableExpressErrorHandler` is truthy * Updating options interface and definitions * Testing express error handler * Test spec fixes * Fix test
* Propagate error to express handler in all situations * Call the default error handler if `enableExpressErrorHandler` is truthy * Updating options interface and definitions * Testing express error handler * Test spec fixes * Fix test
* Propagate error to express handler in all situations * Call the default error handler if `enableExpressErrorHandler` is truthy * Updating options interface and definitions * Testing express error handler * Test spec fixes * Fix test
This PR is to enable the default express error handler on all cases if config param
enableExpressErrorHandleris truthyI'm solving the issue #4696 without changing the current default behavior.