strongloop / strong-remoting

Communicate between objects in servers, mobile apps, and other servers.
www.strongloop.com
Other
105 stars 93 forks source link

[SEMVER-MAJOR] Implement strong error handler for rest-adapter #302

Closed davidcheung closed 8 years ago

davidcheung commented 8 years ago

Using new strong-error-handler for rest-adapter

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

davidcheung commented 8 years ago

@bajtos PTAL

bajtos commented 8 years ago

@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 commented 8 years ago

@davidcheung reviewed.

davidcheung commented 8 years ago

@bajtos PTAL again

bajtos commented 8 years ago

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

davidcheung commented 8 years ago

Thanks @bajtos! I have addressed your comments PTAL

bajtos commented 8 years ago

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

@davidcheung this depends on https://github.com/strongloop/strong-error-handler/pull/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 commented 8 years ago

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

@davidcheung this depends on strongloop/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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@slnode test please

davidcheung commented 8 years ago

should land with https://github.com/strongloop/loopback/pull/2375 when ready

bajtos commented 8 years ago

@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 commented 8 years ago

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

bajtos commented 8 years ago

LGTM

Amir-61 commented 8 years ago

@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 commented 8 years ago

@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