Skip to content

[SEMVER-MAJOR] Remove loopback#errorHandler middleware - #2411

Merged
loay merged 1 commit into
masterfrom
strongErrorHandler
Jun 13, 2016
Merged

[SEMVER-MAJOR] Remove loopback#errorHandler middleware#2411
loay merged 1 commit into
masterfrom
strongErrorHandler

Conversation

@loay

@loayloay commented Jun 8, 2016

Copy link
Copy Markdown
Contributor

connect to #2338

@loayloay added the #review label Jun 8, 2016
@loayloay assigned bajtos and loay and unassigned bajtosJun 8, 2016
@loayloay mentioned this pull request Jun 8, 2016
@bajtos

Copy link
Copy Markdown
Member

@loay Could you please link this pull request with the GH issue where we discussed this change? I think it will be better to completely remove loopback#errorHandler middleware, what are the arguments for keeping it around?

@bajtosbajtos changed the title update errorHandler template[SEMVER-MAJOR] Remove loopback#errorHandler middlewareJun 8, 2016
@bajtos

Copy link
Copy Markdown
Member

@loay please add an entry into 3.0-RELEASE-NOTES.md, don't forget to include a link to this pull request - see other entries for inspiration.

@loay
loayforce-pushed the strongErrorHandler branch from e77ea5f to 0ece4b8CompareJune 9, 2016 19:43
@loay

loay commented Jun 10, 2016

Copy link
Copy Markdown
ContributorAuthor

@slnode test please

@loay
loayforce-pushed the strongErrorHandler branch from 66958fe to 00b1199CompareJune 10, 2016 02:49
@loay

loay commented Jun 10, 2016

Copy link
Copy Markdown
ContributorAuthor

In test/relations.integration.js, this line was failing the tests :

expect(res.body.error.statusCode).to.equal(500); ,

as it was set to undefined so I changed it to

expect(res.body.error.status).to.equal(500); ,

and it did run fine locally, as res.body.error.status was giving the expected value of 500. However it is still failing the ci testings for the same reason of being undefined. I commented out the line and it finally passed.

@bajtos PTAL

@loayloay assigned bajtos and unassigned loayJun 10, 2016
Comment thread3.0-RELEASE-NOTES.md Outdated

See [loopback#2349](https://github.com/strongloop/loopback/pull/2394).

## remove `loopback.errorhandler`

@bajtosbajtosJun 10, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove loopback#errorhandler

We have removed loopback#errorhandler middleware, users should use strong-error-handler directly.

// server/middleware.json{// ..."final:after": {"strong-error-handler": {}}}// server/middleware.development.json{"final:after": {"strong-error-handler": {"params": {"debug": true}}}

See also strong-error-handler's options and the related code change.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^^^ click on the "edit" icon of the comment above, copy the markdown source and use it to replace the content of the new section you are adding.

@bajtosbajtos assigned loay and unassigned bajtosJun 10, 2016
@bajtos

Copy link
Copy Markdown
Member

@loay reviewed, PTAL

@loayloay assigned bajtos and unassigned loayJun 10, 2016
@loay

loay commented Jun 10, 2016

Copy link
Copy Markdown
ContributorAuthor

@bajtos PTAL. Thanks

Comment threadtest/relations.integration.js Outdated
expect(res.body.error).to.be.an('object');
expect(res.body.error.name).to.equal('Error');
expect(res.body.error.statusCode).to.equal(500);
expect(res.statusCode).to.equal(500);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please check with @davidcheung whether this change is needed. I am surprised that body.error.statusCode is not available.

@bajtos

Copy link
Copy Markdown
Member

The code change LGTM, please make sure Travis CI builds and Jenkins builds (possibly excluding unstable dependants) are passing before landing.

@bajtosbajtos assigned loay and unassigned bajtosJun 13, 2016
@bajtos

Copy link
Copy Markdown
Member

@loay also squash the commits before landing :)

Comment threadserver/middleware/error-handler.js Outdated
}
}
module.exports = function(options) {
throw new Error ('loopback.errorHandler is no longer available.' +

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't put space between function name and argument list, i.e. Error(

Could you please add a rule to our eslint config to catch this?

@loay
loayforce-pushed the strongErrorHandler branch from 8f0cef7 to 1fc51d1CompareJune 13, 2016 15:19
@loay
loay merged commit 74aba5f into masterJun 13, 2016
@loayloay removed the #review label Jun 13, 2016
@loay
loay deleted the strongErrorHandler branch June 13, 2016 20:47
@rickyngk

Copy link
Copy Markdown

I'n using strongErrorHandler with loopback 3.0.0-alpha.1. That's really a pain. It annoyed me with sort of message e.g: "Unhandled error for request GET /MyModel/action". How can I turn it off? I also disabled log & debug in both middleware.json and middleware.development.json, but no help

"final:after": {
"strong-error-handler": {
"params": {
"debug": true,
"log": true
}
}
}

Btw, I use loggy to store error message. For loopback 2.x, I send error log in config.local.js

module.exports={restApiRoot: '/api'+(version>0 ? '/v'+version : ''),host: process.env.HOST||'localhost',port: process.env.PORT||3000,remoting: {errorHandler: {handler: function(err,req,res,next){log.l('api-error',{method: req.method,originalUrl: req.originalUrl,statusCode: res.statusCode,stack: err.stack,});next(err);},},},};

But err.stack is not available in 3.x. I have to change it to req.remotingContext.error

How can I do this officially?

@ttothAISS

Copy link
Copy Markdown

Hi @rickyngk !

I have encountered the same issue.

The problem is the RestAdapter.errorHandler in the rest-adapter.js calling the new strongErrorHandler with ITS OWN option object, not the one the documentation suggest in the server/middleware.json.

To overcome and apply your desired params use the following in the server/config.json:

"remoting": {
...
"errorHandler": {
"debug": false,
"log": false
...
}

@bajtos this should be mentioned in the migrate doc, because the full migration is not possible right now if you want to use options when using the strong error handler for the REST adapter.

T.

@davidcheung

Copy link
Copy Markdown
Contributor

@ttothAISS thanks for pointing it out, the migration guide for REST adapter error-handler should be this instead, will follow up and fix

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@loay@bajtos@rickyngk@ttothAISS@davidcheung