add types and introduce writeErrorToResponse by shimks · Pull Request #77 · loopbackio/strong-error-handler

shimks · 2018-08-08T17:30:15Z

Description

related to loopbackio/loopback-next#1598

Related issues

connect to <link_to_referenced_issue>

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style
guide

shimks · 2018-08-08T18:28:05Z

Downstream failure has to do with linting afaik

virkt25 · 2018-08-08T19:06:21Z

 };
 };
+
+module.exports.buildResponseData = buildResponseData;


This should be exported from /lib/data-builder.js and not this file which is just importing in the data-builder.

bajtos · 2018-08-13T09:54:38Z

See my loopbackio/loopback-next#1598 (comment). I think we need to expose a slightly larger functionality.

It would be also great to add a .d.ts file describing the public API of strong-error-handler, so that we can consume this module in LB4 in a type-safe way.

shimks

Remind me to add in documentation please

bajtos · 2018-08-14T09:27:30Z

+ err: Error,
+ req: Express.Request,
+ res: Express.Response
+) => void | Socket;


I believe the handler function is accepting four arguments including a callback, this is an Express requirement. Also the handler should not be returning Socket. If it does, then it's a bug in the implementation. Conceptually, the handler returns void and may invoke next.
exporttypestrongErrorHandler=(err: Error,req: Express.Request,res: Express.Response,next: (err?: any)=>void,)=>void;

bajtos · 2018-08-14T09:28:30Z

@@ -0,0 +1,14 @@
+import * as Handler from './types/handler';
+import * as Options from './types/options';


To be honest, having multiple .d.ts files feel like overkill to me, considering that our typings are less than 30 lines of code in total. Not a big deal though, if you prefer this way then I am ok with that.

bajtos · 2018-08-14T09:29:27Z

+
+export = errorHandler;
+
+declare function errorHandler(


Let's call this errorHandlerFactory because it is a factory function returning back a middleware handler.

bajtos · 2018-08-14T09:31:47Z

+ options?: Options.ErrorHandlerOptions
+): errorHandler.StrongErrorHandler
+
+declare namespace errorHandler {


Do we need this namespace? Isn't it enough to export Handler under the name ErrorHandler?
import{HandlerasErrorHandler}from'./types/handler';import{ErrorHandlerOptions}from'./types/options';export{ErrorHandler,ErrorHandlerOptions};

Your code would have worked if exported functions were properties of module.exports. Instead, our module exports createStrongErrorHandler as module.exports itself, so the export above won't work.
I don't understand everything here (like the export = errorhandler syntax) but I've managed to get it to work as expected following this template: https://www.typescriptlang.org/docs/handbook/declaration-files/templates/module-function-d-ts.html

bajtos

LGTM 👍

bajtos · 2018-08-23T14:24:06Z

+http
+ .createServer((req, res) => {
+ if (errShouldBeThrown) {
+ writeErrorToResponse(new Error('something went wrong'), req, res);


Please show how to pass options too (via the fourth argument).

Please show how to pass options too (via the fourth argument).
☝️

bajtos · 2018-08-23T14:27:09Z

Please check why a downstream build of strong-remoting@master is failing (details). We don't want to introduce a regression.

bajtos · 2018-08-23T14:34:48Z

+ */
+ interface ErrorHandlerOptions {
+ debug?: boolean;
+ log?: boolean;


Should we split this interface into two smaller interfaces, one containing the options used by both the middleware and the writer, the other adding options specific to the middleware (i.e. log)?
interfaceErrorHandlerCoreOptions{debug?: boolean;// etc.}interfaceErrorHandlerMiddlewareOptionsextendsErrorHandlerCoreOptions{log?: boolean;}

Are they passed in as two separate objects? Since both are optional properties I think a single *Options object is better.

Well, the log property is not supported by writeErrorToResponse. Ideally, the interface used by writeErrorToResponse options should not offer properties that are not supported by the function.
declarefunctionerrorHandlerFactory(options?: errorHandlerFactory.ErrorHandlerMiddlewareOptions : errorHandlerFactory.StrongErrorHandler;functionwriteErrorToResponse(err: Error,req: Express.Request,res: Express.Response,options?: errorHandlerFactory.ErrorHandlerCoreOptions): void;
On the second thought, I think the following interface names may be better because they are shorter:
ErrorHandlerOptions (used by middleware factory)
ErrorWriterOptions (used by writeErrorToResponse)
Anyhow, this is not a big deal, I am ok with the current version where there is a single interface only.

virkt25 · 2018-08-24T03:21:21Z

strong-remoting is failing due to linting errors (all the tests pass). I've opened a PR to fix the linting issues strongloop/strong-remoting#452

shimks · 2018-08-29T17:33:11Z

@slnode test please

b-admike

👍

jannyHou

👍 LGTM and the same suggestion as Miroslav post in the README.md file.

bajtos

I have two minor comments to consider.

No further review is necessary as far as I am concerned.

bajtos · 2018-08-30T10:40:27Z

+http
+ .createServer((req, res) => {
+ if (errShouldBeThrown) {
+ writeErrorToResponse(new Error('something went wrong'), req, res);


Please show how to pass options too (via the fourth argument).
☝️

bajtos · 2018-08-30T10:44:49Z

+ /**
+ * Error-handling middleware function. Includes server-side logging
+ */
+ type StrongErrorHandler = (


Can we move types to the top level please, so that users don't have to prefix their usage with errorHandlerFactory namespace?
I am referring to the following types:
StrongErrorHandler
ErrorWriterOptions
ErrorHandlerOptions
Of course, writeErrorToResponse must stay inside errorHandlerFactory namespace, because that's how the JavaScript code is structured.
declareinterfaceErrorWriterOptions{// ...}declareinterfaceErrorHandlerOptions{// ...}declaretypeStrongErrorHandler=// ...declarefunctionerrorHandlerFactory// ...declarenamespaceerrorHandlerFactory{functionwriteErrorToResponse// ...}

If I understand correctly, moving the types outside of the namespace does not allow us to export the types since module.exports is only the handler function itself.
As long as the error handler function isn't used from the module, the users shoudn't have to use the namespace if that was what you were concerned about. See here https://github.com/strongloop/loopback-next/blob/4e180b286bf6a353067995795a7770522332588a/packages/rest/src/providers/reject.provider.ts#L10

If I understand correctly, moving the types outside of the namespace does not allow us to export the types since module.exports is only the handler function itself.
I see what you mean now! I was not aware of this limitation, thank you for explaining it to me.

shimks requested review from bajtos and zbarbuto as code owners August 8, 2018 17:30

shimks force-pushed the export-build-response-data branch from a00e706 to 237bb8aCompare August 8, 2018 17:37

virkt25 reviewed Aug 8, 2018
View reviewed changes

shimks commented Aug 13, 2018
View reviewed changes

bajtos reviewed Aug 14, 2018
View reviewed changes

shimks force-pushed the export-build-response-data branch 3 times, most recently from 5bf7a10 to 6775c04Compare August 14, 2018 18:02

shimks changed the title ~~export buildResponseData~~add types and introduce writeErrorToResponseAug 14, 2018

shimks force-pushed the export-build-response-data branch from a84afee to 4007a1eCompare August 14, 2018 21:06

bajtos approved these changes Aug 23, 2018
View reviewed changes

bajtos reviewed Aug 23, 2018
View reviewed changes

shimks mentioned this pull request Aug 24, 2018
fix(rest): pass a better detailed error message into the response body loopbackio/loopback-next#1598
Merged
7 tasks

b-admike approved these changes Aug 29, 2018
View reviewed changes

jannyHou approved these changes Aug 30, 2018
View reviewed changes

bajtos approved these changes Aug 30, 2018
View reviewed changes

Add type definition and writeErrorToResponse
80ebf30

shimks force-pushed the export-build-response-data branch from cad039c to 80ebf30Compare August 30, 2018 16:09

shimks merged commit 4e29906 into masterAug 30, 2018

shimks deleted the export-build-response-data branch August 30, 2018 16:53

		@@ -0,0 +1,14 @@
		import * as Handler from './types/handler';
		import * as Options from './types/options';

Uh oh!

Conversation

shimks commented Aug 8, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Checklist

Uh oh!

shimks commented Aug 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Aug 13, 2018

Uh oh!

shimks left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos left a comment

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 Aug 23, 2018

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!

virkt25 commented Aug 24, 2018

Uh oh!

shimks commented Aug 29, 2018

Uh oh!

b-admike left a comment

Choose a reason for hiding this comment

Uh oh!

jannyHou left a comment

Choose a reason for hiding this comment

Uh oh!

bajtos left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shimks commented Aug 8, 2018•
edited
Loading