Add callback to make key gen async #7
Conversation
|
nice one! would you mind adding documentation as well? |
|
Nope! I'll get that up right now. |
|
hmm, so this actually only runs off the event loop if you're on a browser, right? i don't see a child_process.spawn in there. if that's the case, we should make sure to document that as well. |
|
also, if this only works in webworkers, we would be easier off just requiring the user to use keypair inside a webworker |
|
When is this pull likely to be merged? |
| }; | ||
| } | ||
|
|
||
| var keypair = forge.rsa.generateKeyPair(null, null, opts, wrappedCallback); |
There was a problem hiding this comment.
Either null should be undefined or the definition of generateKeyPair should check these parameters for null as well as undefined (https://github.com/juliangruber/keypair/blob/master/index.js#L4310 and https://github.com/juliangruber/keypair/blob/master/index.js#L4313). Otherwise, I don't think the default values for these options will be set correctly.
There was a problem hiding this comment.
(Also, should https://github.com/juliangruber/keypair/blob/master/index.js#L4311 and https://github.com/juliangruber/keypair/blob/master/index.js#L1848 default the key size to 2048 rather than 1024?)
|
Keep in mind that spawning a child process is contextually time consuming as well and could block more than simply calculating the entropy |
When all the concerns are addressed. See my two comments above yours |
|
@jimmiehansson @juliangruber I definitely don't have the bandwidth to keep moving this along. If somebody wants to jump in, is there a way to change ownership of a PR? |
|
Afaik they need to create a new one :| But we can link to this one to keep the context |
This will allow the caller to pass in a callback so it doesn't block the event loop while generating the key. Pretty simple change, anything else you'd like me to add?