NODEJS-375 Allow async creation of Uuid #238
Conversation
|
Hi @guzmo, thanks for your contribution! In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. Sincerely, |
|
Thank you @guzmo for signing the Contribution License Agreement. Cheers, |
|
The Synchronous function might be faster for creating a small amount of bytes ( which Uuid is doing I think ), but it might be safer to use the async because you might block the node process. Well, your choice if you want to have it. |
|
As discussed in the ticket, I think is a good idea to have an asynchronous overload of About |
|
@jorgebay Can we create separate tickets for TimeUuid? :) One step at a time is usually a good thing and would be nice to at least get this fix in an upcoming release. Cheers |
|
ok, I've created a separate ticket for I'll talk with the rest of the team to see if we can include it for the upcoming v3.3.0 today and let you know. |
|
We will try to include it for v3.3.0 release. |
lib/types/uuid.js
Outdated
| @@ -108,7 +129,10 @@ function getHex (uuid) { | |||
| * @private | |||
| * @returns {Buffer} | |||
| */ | |||
| function getRandomBytes() { | |||
| function getRandomBytes (cb) { | |||
| if (cb) { | |||
There was a problem hiding this comment.
Can be simplified without the if block, into return crypto.randomBytes(16, cb)
test/unit/uuid-tests.js
Outdated
| it('should generate v4 uuids that do not collide', function (done) { | ||
| var values = {}; | ||
| var length = 100000; | ||
| var count = length; |
There was a problem hiding this comment.
Use built-in utils.times() (same signature as caolan's async.times()), ie:
var utils = require('../../lib/utils');
utils.times(length, function eachTime(n, next) {
Uuid.random(function (err, val) {
if (err) {
return next(err);
}
values[val.toString()] = true;
next();
});
}, function finish(err) {
assert.ifError(err);
assert.strictEqual(Object.keys(values).length, length);
done();
});
|
Good catches! Will fix the review comments tonight. |
|
No problem, glad to help :) |
I was supposed to create a small benchmark of async vs sync but I've had no time :( I ran some small tests with https://www.npmjs.com/package/loadtest, but only on my computer, which are running a lot of other processes at the same time, so hard to tell.
Hope the code is not too messy ;)