[SEMVER-MAJOR] Implement strong error handler for rest-adapter by davidcheung · Pull Request #302 · strongloop/strong-remoting

davidcheung · 2016-05-16T17:09:01Z

Using new strong-error-handler for rest-adapter

connect to https://github.com/strongloop-internal/scrum-loopback/issues/852

3.0-RELEASE-NOTES.md

davidcheung · 2016-05-16T17:21:02Z

    });

-    it('should disable stack trace', function(done) {
+    it.skip('should disable stack trace', function(done) {


this test is for process.env.NODE_ENV = 'production';
do we still want to support this?

No, please remove this test.

davidcheung · 2016-05-18T04:42:10Z

@bajtos PTAL

bajtos · 2016-05-18T14:49:03Z

@davidcheung I added two commits improving release notes, PTAL 751e708, 40e3d04. Feel free to squash them into your commit and dropping my authorship along the way.

bajtos · 2016-05-18T14:50:34Z

+You can learn more about the rationale behind the new handler in
+[this comment](https://github.com/strongloop/loopback/issues/1650#issuecomment-161920555)
+
+User can specific options in their applications in `config.json` as follow:


Are you sure about this? I think a better option is to configure strong-remoting's error handler via server/middleware.json, but we may not be there yet.

{ "routes": { "loopback#rest": { "params": { "debug": true, "log": true } } }

hmm it works in my sand-box
i think the behavior comes from

strong-remoting/lib/rest-adapter.js

Line 315 in aa391d2

root.use(RestAdapter.errorHandler(this.remotes.options.errorHandler));

maybe we can update this when we change it to use middleware.json?

We are already using middleware.json. But never mind, let's leave this out of scope of this pull request and stay consistent with the current state of affairs (i.e. configure it via server/config.json).

bajtos · 2016-05-18T15:02:08Z

@davidcheung reviewed.

davidcheung · 2016-05-19T15:14:49Z

@bajtos PTAL again

bajtos · 2016-05-19T15:58:45Z

+  "restApiRoot": "/api",
+  "host": "0.0.0.0",
+  "port": 3000,
+  "errorHandler": {


One more question though, I think errorHandler should be nested under remoting, could you please check?

{ "restApiRoot": "/api", "remoting": { "errorHandler": { "debug": true } } }

you're right, the one I have was wrong, fixed.

bajtos · 2016-05-19T16:08:03Z

Looks mostly good, I left two more comments to address.

davidcheung · 2016-05-20T14:26:10Z

Thanks @bajtos! I have addressed your comments PTAL

bajtos · 2016-05-20T14:50:30Z

+    }
+  }
+}
+```


Please add a link to strong-error-handler GH repository, so that people can easily find description of the config options available.

Ah, I see the link is already included below, ignore this comment.

bajtos · 2016-05-20T14:54:53Z

One nitpick, the rest LGTM. No further review is necessary.

@davidcheung this depends on loopbackio/strong-error-handler#11, doesn't it? Please wait with landing this patch until that required PR is landed first and CI builds are green on our Jenkins. (The jobs on Travis will keep failing because strong-error-handler is not published to npmjs.org yet.)

davidcheung · 2016-05-20T15:04:14Z

yes what should i do with the strong-error-handler version on package.json?

@davidcheung this depends on loopbackio/strong-error-handler#11, doesn't it? Please wait with landing this patch until that required PR is landed first and CI builds are green on our Jenkins. (The jobs on Travis will keep failing because strong-error-handler is not published to npmjs.org yet.)

bajtos · 2016-05-20T15:05:24Z

yes what should i do with the strong-error-handler version on package.json?

Ah, I overlooked that line, please reset it to ^1.0.0. I'll release 1.0.0 when your strong-error-handler pull request is landed.

davidcheung · 2016-05-24T14:07:13Z

Please do not merge this yet, my loopback-core module is failing locally but the ci passed
will probably need to submit another PR to loopback core before this should be merged.

davidcheung · 2016-05-24T15:39:13Z

@slnode test please

davidcheung · 2016-05-24T21:21:38Z

should land with strongloop/loopback#2375 when ready

bajtos · 2016-06-01T13:45:09Z

@davidcheung could you please add a note into release notes that error.status property is no longer available and clients should use error.statusCode instead?

davidcheung · 2016-06-06T20:06:17Z

Placeholder: https://github.com/strongloop/loopback-datasource-juggler/blob/b039b51610b5ad14b5abdc5bb9e798baa7c70588/lib/dao.js#L1199
is still using error.status

bajtos · 2016-06-07T11:30:48Z

LGTM

Amir-61 · 2016-06-07T19:48:39Z

@davidcheung Did this patch pass CI? Does this patch require loopback-PR#2375? The reason I'm asking is my PR in loopback#2316 was passing, but it fails with lots of failures now with this commit in strong-remoting; I assume loopback-PR#2375 needs to be landed to fix these failures?

Thanks!

davidcheung · 2016-06-07T20:02:23Z

@Amir-61 both PRs depend on each other to pass, I'm currently waiting for CI to run for loopback-PR#2375, once i merge that both sides should pass

davidcheung self-assigned this May 16, 2016

davidcheung added breaking-change #review labels May 16, 2016

davidcheung reviewed May 16, 2016
View reviewed changes

davidcheung added #wip and removed #review labels May 16, 2016

davidcheung added this to the #Epic: Production Error Handler milestone May 16, 2016

davidcheung changed the title ~~[WIP] Implement strong error handler for rest-adapter~~ [SEMVER-MAJOR] Implement strong error handler for rest-adapter May 18, 2016

davidcheung added #review and removed #wip labels May 18, 2016

bajtos reviewed May 18, 2016
View reviewed changes

davidcheung force-pushed the implement-strong-error-handler branch from a459390 to 4e1e6a0 Compare May 19, 2016 03:42

davidcheung assigned bajtos and unassigned davidcheung May 19, 2016

bajtos reviewed May 19, 2016
View reviewed changes

bajtos assigned davidcheung and unassigned bajtos May 19, 2016

davidcheung force-pushed the implement-strong-error-handler branch from 4e1e6a0 to 41bf6f7 Compare May 20, 2016 14:24

davidcheung removed their assignment May 20, 2016

davidcheung assigned bajtos May 20, 2016

bajtos reviewed May 20, 2016
View reviewed changes

bajtos assigned davidcheung and unassigned bajtos May 20, 2016

davidcheung force-pushed the implement-strong-error-handler branch from 41bf6f7 to 27ba7b3 Compare May 20, 2016 15:47

davidcheung mentioned this pull request May 31, 2016

[SEMVER-MAJOR] Fix remoting strong-error-handler strongloop/loopback#2375

Merged

davidcheung force-pushed the implement-strong-error-handler branch from a42ca79 to aa7cb47 Compare June 6, 2016 17:30

davidcheung assigned bajtos and unassigned davidcheung Jun 6, 2016

bajtos assigned davidcheung and unassigned bajtos Jun 7, 2016

rest-adapter to use strong-error-handler

5d0dcd2

davidcheung force-pushed the implement-strong-error-handler branch from 194cb82 to 5d0dcd2 Compare June 7, 2016 14:51

davidcheung merged commit 73e7855 into master Jun 7, 2016

davidcheung deleted the implement-strong-error-handler branch June 7, 2016 15:22

davidcheung removed the #review label Jun 7, 2016

Amir-61 mentioned this pull request Jun 7, 2016

Expose Replace* methods strongloop/loopback#2316

Merged

davidcheung mentioned this pull request Jun 10, 2016

add strongErrorHandler configuration #296

Closed

Uh oh!

Conversation

davidcheung commented May 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidcheung commented May 18, 2016

Uh oh!

bajtos commented May 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented May 18, 2016

Uh oh!

davidcheung commented May 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidcheung May 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented May 19, 2016

Uh oh!

davidcheung commented May 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented May 20, 2016

Uh oh!

davidcheung commented May 20, 2016

Uh oh!

bajtos commented May 20, 2016

Uh oh!

davidcheung commented May 24, 2016

Uh oh!

davidcheung commented May 24, 2016

Uh oh!

davidcheung commented May 24, 2016

Uh oh!

bajtos commented Jun 1, 2016

Uh oh!

davidcheung commented Jun 6, 2016

Uh oh!

bajtos commented Jun 7, 2016

Uh oh!

Amir-61 commented Jun 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidcheung commented Jun 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davidcheung commented May 16, 2016 •

edited

Loading

davidcheung May 20, 2016 •

edited

Loading

Amir-61 commented Jun 7, 2016 •

edited

Loading