-
-
Notifications
You must be signed in to change notification settings - Fork 902
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
fix: remove v4 options default assignment preventing native.randomUUID from being used #786
Conversation
…s, pull uuidjs#763 preventing native.randomUUID being used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
This fix looks good. I've manually verified that it works, but it'd be really nice to have a unit test for this. That's a little challenging because there's no simply way to shim the crypto.randomUUID()
method, however. So, instead, can you make the following changes:
diff --git a/src/test/v4.test.ts b/src/test/v4.test.ts
index 36574f3..c04b8fa 100644
--- a/src/test/v4.test.ts
+++ b/src/test/v4.test.ts
@@ -1,6 +1,6 @@
import * as assert from 'assert';
import test, { describe } from 'node:test';
-import v4 from '../v4.js';
+import v4, { testingOnlyNativeCalls } from '../v4.js';
const randomBytesFixture = Uint8Array.of(
0x10,
@@ -48,6 +48,12 @@ describe('v4', () => {
assert.ok(id1 !== id2);
});
+ test('uses native randomUUID()', () => {
+ const nativeCallsBefore = testingOnlyNativeCalls;
+ v4();
+ assert.equal(testingOnlyNativeCalls, nativeCallsBefore + 1);
+ });
+
test('explicit options.random produces expected result', () => {
const id = v4({ random: randomBytesFixture });
assert.strictEqual(id, '109156be-c4fb-41ea-b1b4-efe1671c5836');
diff --git a/src/v4.ts b/src/v4.ts
index 3370e55..7e7d9d7 100644
--- a/src/v4.ts
+++ b/src/v4.ts
@@ -3,10 +3,14 @@ import native from './native.js';
import rng from './rng.js';
import { unsafeStringify } from './stringify.js';
+export let testingOnlyNativeCalls = 0;
+
function v4(options?: Version4Options, buf?: undefined, offset?: number): string;
function v4(options?: Version4Options, buf?: Uint8Array, offset?: number): Uint8Array;
function v4(options?: Version4Options, buf?: Uint8Array, offset?: number): UUIDTypes {
if (native.randomUUID && !buf && !options) {
+ // Track number of calls to native randomUUID() for testing purposes
+ testingOnlyNativeCalls++;
return native.randomUUID();
}
@broofa I've added some tests that I think are cleaner than adding a variable inside the main code and counting that! But there's a minor issue with these tests, as you can see here:
the first condition is for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last change and I think we'll be ready to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving and merging. I'll make the suggested type changes in a separate PR.
Thanks for the contribution!
Sorry I was a bit busy, thanks for responding and being open to PRs |
This removes the v4 options default assignment introduced during porting to ts (by mistake maybe?), during #763 - PR diff - commit preventing
native.randomUUID
from being usedif
options
always have a value, we will never enter the if in line 11.{}
will be assigned tooptions
in line 15 after the native checkit looks to me that the line 9 statement is added by mistake