Skip to content

added async signing - #127

Merged
jfromaniello merged 1 commit into
auth0:masterfrom
lukewestby:master
Oct 2, 2015
Merged

jfromaniello merged 1 commit into
auth0:masterfrom
lukewestby:master

Conversation

@lukewestby

Copy link
Copy Markdown

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

@syzer

syzer commented Sep 28, 2015

Copy link
Copy Markdown

👍

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

Copy link
Copy Markdown
Contributor

Thank you! pulished as v5.1.0

@syzer

syzer commented Oct 2, 2015

Copy link
Copy Markdown

awesome!!! 👍

@lukewestby

Copy link
Copy Markdown
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

Nepoxx commented Nov 23, 2015

Copy link
Copy Markdown

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
Copy Markdown
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

Nepoxx commented Nov 24, 2015

Copy link
Copy Markdown

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