Skip to content

Rewrite of the Readme#62

Open
jakoblorz wants to merge 5 commits into
hokaccha:masterfrom
jakoblorz:master
Open

Rewrite of the Readme#62
jakoblorz wants to merge 5 commits into
hokaccha:masterfrom
jakoblorz:master

Conversation

@jakoblorz
Copy link
Copy Markdown

Hi,
I just thought I had to update the readme because the structure was not that easy to follow (it was in no way bad but I think it might be better now). I have no problem if this pull request is declined, as it is more an "offer" 😄 . Greetings, Jakob Lorz

Copy link
Copy Markdown
Collaborator

@alexjab alexjab left a comment

Choose a reason for hiding this comment

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

I think you did great, this module does deserves a better documentation ;-)

I put a couple of comments (most of them very subjective and personal, so don't feel personally attacked), in any case, we'll need @hokaccha 's opinion.

Also, feel free to cherry-pick my comments.

Comment thread README.md
# node-jwt-simple
<div align="center">
<img src="https://jwt.io/assets/logo.svg"><br><br>
</div>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not a huge fan of using this external resource, we do not want people to think that jwt.io (which is maintained by auth0) endorses this module in any way.

Moreover, this looks like marketing material, and I don't think we need this.

Comment thread README.md

[JWT(JSON Web Token)](http://self-issued.info/docs/draft-jones-json-web-token.html) encode and decode module for node.js.
# Welcome
Secure your application in seconds using standardised **JWT-Tokens**. [RFC 7519](https://tools.ietf.org/html/rfc7519)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This awfully looks like marketing as well. I would go for something more neutral, such as:

This module allows you to implement authentication in your web applications using JSON Web Tokens (aka JWT). JWTs are standardized in [RFC 7519](https://tools.ietf.org/html/rfc7519).

Comment thread README.md

## Sample Use-Case
You may use this as an authentication-method for your API: on one route you encode a token, storing the authenticated user (e.g. after checking password and email) in it. On all other routes you check if the token is valid - this indicates that the user once did authenticate himself successfully.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not a huge fan of this kind of introductions, this looks not necessary here, but that is just my opinion.

Comment thread README.md

## Usage
### Encoding
Encode a Object into your token - the `jwt.encode` function takes a maximum of 3 parameters:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Since the parameters are almost self-explanatory, here is a proposal:

### jwt.encode(payload, secret, [algorithm])

Creates a token from a payload. The available algorithms are `HS256` (default), `HS384`, `HS512` and `RS256`.

Example:
...

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Using the signature of the method in the title allows the user to get an idea of the API right from the start, even before reading the doc, which is good !

Comment thread README.md
| token | the token previously generated | No |
| secret | key which previously encoded the token | No |
| noVerify | turn off verification **ON YOUR OWN RISK** | Yes |
| algorithm | select another algorithm. see encode for algorithm options. | Yes, but noVerify must been set before |
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same comment as for encode, I'd try for a simpler way of explaining things:

### jwt.decode(token, secret, [noVerify], [algorithm])
...

Also:

  • I would go for a slightly less violent warning (after all, OS licenses are mostly "at your own risk"), maybe **turn off** verification, or explain what it does,
  • "Yes, but noVerify must been set before" that is the way js works, I don't think you need to tell this to the user.

Comment thread README.md
* jwt.decode(token, key, noVerify, algorithm)
*/
var jwt = require('jwt-simple');
var secret = Buffer.from('fe1a1915a379f3be5394b64d14794932', 'hex');
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Are you sure that this is a Buffer ? The jsdoc says String...

Comment thread README.md
</div>

[JWT(JSON Web Token)](http://self-issued.info/docs/draft-jones-json-web-token.html) encode and decode module for node.js.
# Welcome
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not use full, IMHO.

@hokaccha
Copy link
Copy Markdown
Owner

@jakoblorz Thank you for your good documentation. I totally agree with @alexjab's comments so could you fix them?

@jakoblorz
Copy link
Copy Markdown
Author

jakoblorz commented Feb 19, 2017

@hokaccha @alexjab I will soon, I am currently laying completely flat in my bed due to illness, so expect the resolve in the next 2-3 weeks 💯

Sign up for free to 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.

3 participants