fix: remove v4 options default assignment preventing native.randomUUID from being used#786
fix: remove v4 options default assignment preventing native.randomUUID from being used#786broofa merged 2 commits intouuidjs:mainfrom
Conversation
…s, pull uuidjs#763 preventing native.randomUUID being used
broofa
left a comment
There was a problem hiding this comment.
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 |
broofa
left a comment
There was a problem hiding this comment.
One last change and I think we'll be ready to merge.
broofa
left a comment
There was a problem hiding this comment.
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.randomUUIDfrom being usedif
optionsalways have a value, we will never enter the if in line 11.{}will be assigned tooptionsin line 15 after the native checkit looks to me that the line 9 statement is added by mistake