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

always use jws.sign such that errors can be at all handled #165

Closed
wants to merge 1 commit into from

Conversation

omsmith
Copy link

@omsmith omsmith commented Jan 17, 2016

This PR switches sign to always use jws#sign instead of the streaming interface. Why?

  1. I was going to open a PR to handle error events from the stream, however callback is not an errback (oops!), as such the only way to actually catch/handle errors if for them to throw synchronously - and thus we need to use the more-synchronous method.
  2. The stream interface calls jws#sign anyway, after buffering input data. In this case, all inputs are already known (and small), meaning the streaming interface just adds overhead
  3. Similar to 2, and my sentiment in Return a Promise #111, this is all synchronous anyway, and the callback APIs provide little-to-no value (though atleast verify is an errback!)

@jfromaniello
Copy link
Member

You are right... But I'd rather change the next major version to do be an errback

jws.createSign({
   header: header,
   privateKey: secretOrPrivateKey,
   payload: JSON.stringify(payload)
}).once('done', function (signature) { callback(null, signature); } )
  .once('error', callback); 

does it make sense?

@jfromaniello
Copy link
Member

I will close this in favor of handling #169 in the next major release.

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.

2 participants