Skip to content

Readme cleanup#36

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
crandmck merged 6 commits into from
Jan 9, 2017
Merged

Readme cleanup #36

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3fc9b7b
Cleanup README for docs
crandmck Jan 4, 2017
9cc3d97
Final fixes
crandmck Jan 4, 2017
beca1e8
Update README.md
crandmck Jan 5, 2017
59384ad
Update README.md
crandmck Jan 6, 2017
a202951
Update README.md
crandmck Jan 6, 2017
bd0691e
Update README.md
crandmck Jan 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
186 changes: 113 additions & 73 deletions README.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,37 +1,25 @@
# strong-error-handler

Error handler for use in development (debug) and production environments.
This package is an error handler for use in both development (debug) and production environments.

## Production mode
In production mode, `strong-error-handler` omits details from error responses to prevent leaking sensitive information:

- error responses are purposely left without detail in order to prevent leaking sensitive information.
- For 5xx errors, the output contains only the status code and the status name from the HTTP specification.
- For 4xx errors, the output contains the full error message (`error.message`) and the contents of the `details`
property (`error.details`). The latter is typically used by `ValidationError` to provide machine-readable details
about the validation problems.
property (`error.details`) that `ValidationError` typically uses to provide machine-readable details
about validation problems.

## Debug Mode
In debug mode, `strong-error-handler` returns full error stack traces and internal details of any error objects to the client in the HTTP responses.

- Full error stack traces and internal details of any object passed as the error will be sent back to the client
in the HTTP responses when an error occurs.

## Response Format

- `strong-error-handler` currently supports HTML and JSON responses.
- When the object is a standard Error object, the string provided by the stack property will be returned in HTML/text
responses.
- When the object is a non-Error object, the result of `util.inspect` will be returned in HTML/text responses.
- For JSON responses, the result will be an object with all enumerable properties from the object in the response.

## Install
## Installation

```bash
$ npm install strong-error-handler
$ npm install --save strong-error-handler
```

## Usage
## Use

@bajtosbajtosJan 5, 2017

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.

Nitpick - feel free to ignore:

I think we are using Usage in other places too, and many other npm packages do the same - see e.g. https://www.npmjs.com/package/glob#usage or https://www.npmjs.com/package/chai#usage.

I would prefer to follow this convention and keep using Usage as the section title.

@crandmckcrandmckJan 5, 2017•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I am well aware that lots of documentation uses "usage" in this manner, but I always prefer "use". [1] "Usage" is a favorite word of engineers and programmers because it sounds more sophisticated. In this context and noun form, the two words mean the same thing, and so I prefer the shorter and more common word as a matter of principle. I'm sure there are still instances of "usage" in our docs, but I have and will continue to change it when I run across it.

[1] http://blogs.transparent.com/english/use-versus-usage/

Some people use the word “usage” as though it were just a fancier form of the word “use,” but this is not the best way to use the word. In general, if either “use” or “usage” seems like it could work in a sentence the best choice is probably “use” as this is a more commonly used word ...

@bajtosbajtosJan 6, 2017

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.

Haha, looks like I may have almost started a grammar war here. Thank you for explaining me the reasoning, I am fine with this change then. Can you perhaps emphasis this convention in our documentation for contributors, e.g. in http://loopback.io/doc/en/contrib/README-guidelines.html#basic-use

@crandmckcrandmckJan 6, 2017•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good suggestion. The heading of that section "Basic use" is mean to be the heading in the README, but that's not clear (we used to have a README template that made it clearer). In addition, this kind of thing should go into http://loopback.io/doc/en/contrib/Doc-style-guide.html#spelling-grammar-and-usage as well.

I realize now that in general I need to do some work on our doc style guide, which I've mostly let slide. The whole point of having a style guide is to avoid rehashing such points, so this is a good prod for me to update it...


In an express-based application:
In an Express-based application:

```js
var express = require('express');
Expand All@@ -49,90 +37,129 @@ app.use(errorHandler({
app.listen(3000);
```

In LoopBack applications, add the following entry to your
`server/middleware.json` file.
In LoopBack applications, add the following entry to `server/middleware.json`:

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

## Content Type
In general, `strong-error-handler` must be the last middleware function registered.

Depending on the request header's `Accepts`, response will be returned in
the corresponding content-type, current supported types include:
- JSON (`json`/`application/json`)
- HTML (`html`/`text/html`)
The above configuration will log errors to the server console, but not return stack traces in HTTP responses.
For details on configuration options, see below.

*There are plans to support other formats such as Text and XML.*
### Response format and content type

## Options
The `strong-error-handler` package supports HTML and JSON responses:

#### debug
- When the object is a standard Error object, it returns the string provided by the stack property in HTML/text
responses.
- When the object is a non-Error object, it returns the result of `util.inspect` in HTML/text responses.
- For JSON responses, the result is an object with all enumerable properties from the object in the response.

`boolean`, defaults to `false`.
The content type of the response depends on the request's `Accepts` header.

If you need to set the environment to development mode, you will need to change the value to `true`.
- For Accepts header `json` or `application/json`, the response content type is JSON.
- For Accepts header `html` or `text/html`, the response content type is HTML.

When enabled, HTTP responses include all error properties, including
sensitive data such as file paths, URLs and stack traces. *See Examples below*
*There are plans to support other formats such as Text and XML.*

#### log
## Options

`boolean`, defaults to `true`.
| Option | Type | Default | Description |

@bajtosbajtosJan 5, 2017

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.

Can we revert back to the old formatting of options please?

This table is difficult to grok in markdown and there is no way how to create a link (URL) pointing to a specific option.

@crandmckcrandmckJan 5, 2017•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I think a table is easier to understand and is our standard format for documenting options, used throughout the docs. (Again, there are probably cases that I haven't gotten around to fixing yet, but they are the exception.)

In general, research shows that tabular formatting of reference information is easier to understand versus narrative format. It's easier to scan and understand how different values relate to each other. To be fair, in this case there are only two properties, so it's not as big a difference.

Tabular format is certainly more compact, and I think if you look at the two formats side-by-side you will see that the table is easier to comprehend:

Table format

table

Narrative format

nontable

(Note that I moved the "customization" info into its own section).

As far as linking to a specific property, this has never come up and no one has asked for it before. IMO it's sufficient to state the property and link to the "Options" section, which is what we do everywhere else.

@bajtosbajtosJan 6, 2017

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.

I understand your arguments for using the table layout. On the other hand, most packages on npm use the narrative format, I think Node.js developers are accustomed to that. Formatting the argument documentation as a table may make it more difficult to read the content, simply because the form is not familiar.

As far as linking to a specific property, this has never come up and no one has asked for it before. IMO it's sufficient to state the property and link to the "Options" section, which is what we do everywhere else.

Well, I have personally run into this issue many times, but never bothered to report it. Mae culpa. I cannot tell how much difficult it is for the readers when the link is in the form of URL + "and go find this property name", so maybe it doesn't really matter.

@crandmckcrandmckJan 6, 2017•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

On the other hand, most packages on npm use the narrative format...

I don't think that's true. There are certainly examples of both formats, but the following use tables for option/parameter reference:

Others: Blueprint, KeystoneJS, etc. etc.

To be clear, I'm lumping API reference doc for method parameters together with doc for JSON options here, but the two are similar enough that it's apt.

There are of course MANY Node packages that do use narrative formatting as well, so I don't think one can claim either is an overwhelming standard.

In any case, LoopBack docs (both on loopback.io and apidocs.strongloop.com) use tables for this type of reference material, so I don't see why this should be an exception.

I have personally run into this issue many times, but never bothered to report it....

Well that's good to know, and it's simple enough to add an anchor in a table row for this purpose as needed (I just tested this and it works). In this particular case, I think it's moot since there are only two options, so linking to the section head is virtually the same as linking to the table row. However, if you want an anchor for each option, I'd be happy to add that, so you could link to README.md#options.log for example.

@bajtosbajtosJan 9, 2017

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.

Well that's good to know, and it's simple enough to add an anchor in a table row for this purpose as needed (I just tested this and it works).

Sounds good!

In this particular case, I think it's moot since there are only two options, so linking to the section head is virtually the same as linking to the table row.

Agreed. We can always add these anchors later as needed.

| ---- | ---- | ---- | ---- |
| debug | Boolean    | `false` | If `true`, HTTP responses include all error properties, including sensitive data such as file paths, URLs and stack traces. See [Example output](#example) below. |
| log | Boolean | `true` | If `true`, all errors are printed via `console.error`, including an array of fields (custom error properties) that are safe to include in response messages (both 4xx and 5xx). <br/> If `false`, sends only the error back in the response. |

When enabled, all errors are printed via `console.error`. That includes an array of fields (custom error properties)
that are safe to include in response messages (both 4xx and 5xx).
### Customizing log format

When not enabled, it only sends the error back in the response.
**Express**

Customization of the log format is intentionally not allowed. If you would like
to use a different format/logger, disable this option and add your own custom
error-handling middleware.
To use a different log format, add your own custom error-handling middleware then disable `errorHandler.log`.
For example, in an Express application:

```js
app.use(myErrorLogger());

@bajtosbajtosJan 5, 2017

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.

What is the reason for moving the error-logger middleware after the error handler? Did you verify that this will work as intended? As far as I understand and remember this topic, errorHandler sends HTTP response and does not call next (see lib/handler.js#L59), therefore any middleware registered after the error handler is not invoked.

@crandmckcrandmckJan 5, 2017•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I did that because that's the order stated in the text, and logically it made sense to me to disable strong-error-handler logging first and then add the custom logger. Obviously, that was a mistake (apologies).

I did not test it, but I have been unsuccessful in getting it to run in either order ....which begs the question: where does this code go? And are there any requirements on the myErrorLogger function? Must it conform to the standard of Express error middleware? Or...?

If it MUST be in the opposite order then we should state so explicitly and also reverse the clauses in the prior sentence:

To use a different log format, add your own custom error-handling middleware and then disable errorHandler.log ...

LMK if that's indeed the case, and I'll make that change.

@davidcheungdavidcheungJan 6, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is the line app.use(errorHandler({ log: false })); implying user is not using a scaffolded Loopback app that comes with it in middleware.json right? i find it a little confusing
especially many users might have a prescaffolded LB app coming here, then adding the 2 lines there and having the error handler registered twice.

Not sure whats the best way to document how to use it for LB app vs Express app, any suggestion?
@bajtos@crandmck

@crandmckcrandmckJan 6, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@davidcheung noted that these lines are for an Express app, not a standard current LB app. If that's true, then we should state that, but more importantly also explain how to set up custom logger for a LB app.

@bajtosbajtosJan 6, 2017

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.

If it MUST be in the opposite order then we should state so explicitly and also reverse the clauses in the prior sentence

Yes please, reverse the order in the text.

I did not test it, but I have been unsuccessful in getting it to run in either order ....which begs the question: where does this code go? And are there any requirements on the myErrorLogger function? Must it conform to the standard of Express error middleware? Or...?

The example code is for an Express app. Since there is no standard convention for structuring Express server file (AFAIK), it's hard to tell where exactly this code should go. In general, strong-error-handler should be the last middleware added to the server, i.e. it should be just before app.listen call.

varapp=express();varerrorHandler=require('strong-error-handler');// add request pre-processing middleware// register your routes// log errorsapp.use(functionmyErrorLogger(err,req,res,next){console.log('unhandled error',err);next(err);});// the ultimate error handlerapp.use(errorHandler({log: false}));app.listen(3000,function(){console.log('Server listening at http://localhost:3000/');});

Not sure whats the best way to document how to use it for LB app

For LoopBack applications, the user need to put the custom error-logging middleware into a standalone file. I was writing an example recently, I wish I knew where it was :( Let me type it again:

// server/middleware/error-logger.js
module.exports = function(options) {
return function logError(err, req, res, next) {
console.log('unhandled error' ,err);
next(err);
};
};
// server/middleware.json
{
// ...
"final:after": {
"./middleware/error-logger": {},
"strong-error-handler": {
"params": {
log: false
}
}
}

Note that the default middleware.development.json file explicitly enables logging in strong-error-handler params, so users should probably change that file too.

@crandmckcrandmckJan 6, 2017•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

OK, thanks for clarifying! I think this is a good clarification, and I added most of the info from your last comment above to the README. PTAL.

NOTE: I didn't have a chance to actually run this code yet, but I'll do so before merging the PR to ensure it works correctly.

app.use(errorHandler({ log: false }));
```

## Migration to strong-error-handler for existing LoopBack applications
In general, add `strong-error-handler` as the last middleware function, just before calling `app.listen()`.

@bajtosbajtosJan 9, 2017

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.

Can we emphasis this fact in the Use section above too?

@crandmckcrandmckJan 9, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Added a note.


1. In package.json dependencies, remove ` "errorhandler": "^x.x.x”,`
1. Run `npm install --save strong-error-handler`
1. In `./server/config.json`, remove:
**LoopBack**

```
"errorHandler": {
"disableStackTrace": false
}
```
and replace it with `"handleErrors": false`.
1. In `server/middleware.json`, remove
For LoopBack applications, put custom error-logging middleware in a separate file; for example, `server/middleware/error-logger.js`:

```
"final:after": {
"loopback#errorHandler": {}
}
```
and replace it with:
```
```
module.exports = function(options) {
return function logError(err, req, res, next) {
console.log('unhandled error' ,err);
next(err);
};
};
```

Then in `server/middleware.json`, specify your custom error logging function as follows:

```
{
// ...
"final:after": {
"strong-error-handler": {}
}
```
1. In the `./server`, delete `middleware.production.json`.
1. In the `./server`, create `middleware.development.json` containing:
"./middleware/error-logger": {},
"strong-error-handler": {
"params": {
log: false
}
}
}
```

The default `middleware.development.json` file explicitly enables logging in strong-error-handler params, so you will need to change that file too.

## Migration from old LoopBack error handler

```
{
NOTE: This is only required for applications scaffolded with old versions of the `slc loopback` tool.

To migrate a LoopBack 2.x application to use `strong-error-handler`:

1. In `package.json` dependencies, remove `"errorhandler": "^x.x.x”,`
1. Install the new error handler by entering the command:
<pre>npm install --save strong-error-handler</pre>
1. In `server/config.json`, remove:
<pre>
"remoting": {
...
"errorHandler": {
"disableStackTrace": false
}</pre>
and replace it with:
<pre>
"remoting": {
...,
"rest": {
"handleErrors": false
}</pre>
1. In `server/middleware.json`, remove:
<pre>
"final:after": {
"loopback#errorHandler": {}
}</pre>
and replace it with:
<pre>
"final:after": {
"strong-error-handler": {}
}</pre>
1. Delete `server/middleware.production.json`.
1. Create `server/middleware.development.json` containing:
<pre>
"final:after": {
"strong-error-handler": {
"params": {
Expand All@@ -141,10 +168,12 @@ app.use(errorHandler({ log: false }));
}
}
}
}
```
</pre>

For more information, see
[Migrating apps to LoopBack 3.0](http://loopback.io/doc/en/lb3/Migrating-to-3.0.html#update-use-of-rest-error-handler).

## Examples:
## Example

Error generated when `debug: false` :

Expand All@@ -159,5 +188,16 @@ Error generated when `debug: true` :
{ statusCode: 500,
name: 'Error',
message: 'a test error message',
stack: 'Error: a test error message\n at Context.<anonymous> (User/strong-error-handler/test/handler.test.js:220:21)\n at callFnAsync (User/strong-error-handler/node_modules/mocha/lib/runnable.js:349:8)\n at Test.Runnable.run (User/strong-error-handler/node_modules/mocha/lib/runnable.js:301:7)\n at Runner.runTest (User/strong-error-handler/node_modules/mocha/lib/runner.js:422:10)\n at User/strong-error-handler/node_modules/mocha/lib/runner.js:528:12\n at next (User/strong-error-handler/node_modules/mocha/lib/runner.js:342:14)\n at User/strong-error-handler/node_modules/mocha/lib/runner.js:352:7\n at next (User/strong-error-handler/node_modules/mocha/lib/runner.js:284:14)\n at Immediate._onImmediate (User/strong-error-handler/node_modules/mocha/lib/runner.js:320:5)\n at tryOnImmediate (timers.js:543:15)\n at processImmediate [as _immediateCallback] (timers.js:523:5)' }}
stack: 'Error: a test error message
at Context.<anonymous> (User/strong-error-handler/test/handler.test.js:220:21)
at callFnAsync (User/strong-error-handler/node_modules/mocha/lib/runnable.js:349:8)
at Test.Runnable.run (User/strong-error-handler/node_modules/mocha/lib/runnable.js:301:7)
at Runner.runTest (User/strong-error-handler/node_modules/mocha/lib/runner.js:422:10)
at User/strong-error-handler/node_modules/mocha/lib/runner.js:528:12
at next (User/strong-error-handler/node_modules/mocha/lib/runner.js:342:14)
at User/strong-error-handler/node_modules/mocha/lib/runner.js:352:7
at next (User/strong-error-handler/node_modules/mocha/lib/runner.js:284:14)
at Immediate._onImmediate (User/strong-error-handler/node_modules/mocha/lib/runner.js:320:5)
at tryOnImmediate (timers.js:543:15)
at processImmediate [as _immediateCallback] (timers.js:523:5)' }}
```