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

added async signing #127

Merged
merged 1 commit into from
Oct 2, 2015
Merged

added async signing #127

merged 1 commit into from
Oct 2, 2015

Conversation

lukewestby
Copy link
Contributor

This PR addresses issue #106. Feedback would be appreciated, especially around whether the unit test is sufficient. Thanks!

@syzer
Copy link

syzer commented Sep 28, 2015

👍

jfromaniello added a commit that referenced this pull request Oct 2, 2015
@jfromaniello jfromaniello merged commit 1fa326f into auth0:master Oct 2, 2015
@jfromaniello
Copy link
Member

Thank you! pulished as v5.1.0

@syzer
Copy link

syzer commented Oct 2, 2015

awesome!!! 👍

@lukewestby
Copy link
Contributor Author

No problem! This library is a cornerstone for me, I'm happy to help.

lukewestby pushed a commit to lukewestby/node-jsonwebtoken that referenced this pull request Oct 2, 2015
@Nepoxx
Copy link

Nepoxx commented Nov 23, 2015

FYI: While this is great, it does not respect the error first callback convention. This unfortunately means that for instance this does not work with Bluebird's promisify or .fromCallback().

@lukewestby
Copy link
Contributor Author

@Nepoxx since this function doesn't have a failure case would you suggest just always passing undefined to the first arg of the callback?

@Nepoxx
Copy link

Nepoxx commented Nov 24, 2015

I'd say null.

Changing this would break backwards compatibility. I think it should be changed to return null/undefined as to be more standard (verify's callback uses error-first), but then again this isn't big issue (for me).

Thoughts?

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