Skip to content
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.

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

Update deps, use npm scripts, add coverage #54

Merged
merged 1 commit into from
Nov 9, 2016

Conversation

DaAwesomeP
Copy link
Contributor

@DaAwesomeP DaAwesomeP commented Oct 2, 2016

  • Fix license declaration in package.json and repo
  • Add Travis compatibility
  • Add coverage
  • Update dependencies
  • Use NPM scripts instead of Make
  • Move to lib folder as was specified in package.json

@DaAwesomeP
Copy link
Contributor Author

@stiang I've noticed that a lot of pull requests have built up that either update the deps or move over to current NodeJS compatible ES6 syntax. Would you like me to make the ES6 edits? Merging this will then be able to close 3 other pulls.

@DaAwesomeP
Copy link
Contributor Author

cc @stiang @tejasmanohar I don't mean to be impatient or picky, but this is a very popular repo (according to NPM), and I feel that it could benefit from some updates.

@@ -101,3 +101,6 @@ function resolveCookies(ctx, opts) {
return ctx.cookies.get(opts.cookie);
}
}

// Export JWT as a convenience
module.exports.jwt = require('jsonwebtoken');
Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

@DaAwesomeP DaAwesomeP Oct 27, 2016

Choose a reason for hiding this comment

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

It's mentioned in the docs: a9353a1#diff-04c6e90faac2675aa89e2176d2eec7d8L190. I can remove it if you'd like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tejasmanohar removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose of the convenience function is to provide the matching JWT version for signing tokens. Otherwise an extra library is required on the user end, and koa-jwt doesn't provide all of the methods necessary to use it.

Copy link
Member

Choose a reason for hiding this comment

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

fair

@DaAwesomeP
Copy link
Contributor Author

This will close #41, #45, and possibly #48.

@tejasmanohar
Copy link
Member

Tejass-MacBook-Pro:jwt tejas$ npm publish
npm ERR! publish Failed PUT 403
npm ERR! Darwin 15.6.0
npm ERR! argv "/usr/local/bin/node" "/usr/local/bin/npm" "publish"
npm ERR! node v6.3.1
npm ERR! npm  v3.10.3
npm ERR! code E403

npm ERR! you do not have permission to publish "koa-jwt". Are you logged in as the correct user? : koa-jwt
npm ERR!
npm ERR! If you need help, you may report this error at:
npm ERR!     <https://github.com/npm/npm/issues>

npm ERR! Please include the following file with any support request:
npm ERR!     /Users/tejas/dev/src/github.com/koajs/jwt/npm-debug.log

@stiang @jonathanong can you publish / give me access to? this pr was sitting open for a while so i wanted to address it but looks like i don't have access

@DaAwesomeP
Copy link
Contributor Author

@tejasmanohar thanks!

@stiang
Copy link
Collaborator

stiang commented Nov 10, 2016

@tejasmanohar Thanks for stepping up! I basically have no time to maintain this at the moment, so it’s great that you take responsibility. I believe I have published the latest changes as 2.1.0 using the koa2 tag. Here are the commands I used:

git co koa-v2
npm publish --tag koa2

Let me know if this didn’t work as expected. As for publishing rights, should I just add you as an owner in npm? What email should I use?

@DaAwesomeP Thanks for the PR and for mildly nagging us to get it merged :) Sorry about the delay!

@tejasmanohar
Copy link
Member

tejasmanohar commented Nov 10, 2016

@stiang You can add me as me@tejas.io or tejasmanohar on NPM, but frankly, I'm also fine without publishing rights, as I barely use Koa or Node anymore so probably am not in a good position to maintain this. It just disappointed me to see no one answering the PR for a while so I did :P

@sdd
Copy link
Collaborator

sdd commented Jan 17, 2017

Hi @stiang, thanks for giving me push rights earlier also. I use node and koa daily across several projects. I'm happy to help out with npm publishing as well if you like, i'm on there as https://www.npmjs.com/~sc0ttyd

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.

4 participants