feat: preserve types of passthrough buffers in v1-v7

[BurningEnlightenment]

The generator APIs accept a pre-allocated buffer and return said instance, but the function type signature didn't encode this information. Therefore we just need to sprinkle a few generics onto them to make it work.

This is mainly useful for nodejs users where Buffer accepting APIs are prevalent. Btw. the overload signatures ensure that the following does not compile:

v1<Buffer>(undefined, undefined)

It would be nice if parse() could optionally accept a buffer constructor, but that requires a bit more thought.

[broofa]

This looks interesting. I don't have time to dig into this in detail at the moment, but I do have a question ...

When I did the TS rewrite, I rather deliberately decided to do away with nodejs-specific types such as Buffer, and ambiguous array types (e.g. Array) and just go with UInt8Array throughout. Doing so felt "clean", in that it makes the code and the API easier to document and reason about. So I'm a little leary of adding support for Buffer types back in.

So I guess the question here is, "Is this really a good idea"? I suspect I know the answer, but I'd like to hear the high-level argument for this from someone other than myself. Or maybe another way to frame this question is, "Does accepting this change require revisiting that high-level decision (of standardizing on Uint8Array)?"

[BurningEnlightenment]

When I did the TS rewrite, I rather deliberately decided to do away with nodejs-specific types such as Buffer, and ambiguous array types (e.g. Array) and just go with UInt8Array throughout. Doing so felt "clean", in that it makes the code and the API easier to document and reason about.

This has absolutely been the right choice as Uint8Array is the universal JS type for manipulating byte arrays. It therefore directly satisfies the interface segregation principle and also caters to the dependency inversion principle. On the other hand

Does accepting this change require revisiting that high-level decision (of standardizing on Uint8Array)?

No, it does not. Note how we never reference anything nodejs specific. We only take care to not perform unnecessary type erasure. <TBuf extends Uint8Array = Uint8Array> still requires our TBuf parameter to be an Uint8Array, i.e. we still only depend on the minimally necessary interface. Buffer from nodejs extends Uint8Array and therefore makes up a valid TBuf type parameter while Array does not. If deno or bun come up with their own Uint8Array subtypes it'll work with them just as well.

This really helps in cases like better-sqlite3 which only works with Buffer instances. Currently I am required to write

this.db
  .prepare('INSERT INTO entry VALUES (?)')
  // takes a dependency on a documented implementation property
  // not expressed in the type system
  .run(v4(undefined, Buffer.alloc(16)) as Buffer);

this.db
  .prepare('INSERT INTO entry VALUES (?)')
  // unnecessary copy
  .run(Buffer.from(v4(undefined, Buffer.alloc(16))));

// technically correct, but eww
const uuid =  Buffer.alloc(16);
v4(undefined,uuid);
this.db
  .prepare('INSERT INTO entry VALUES (?)')
  .run(uuid);

This change enables the intuitive to work:

this.db
  .prepare('INSERT INTO entry VALUES (?)')
  .run(v4(undefined, Buffer.alloc(16)));

// this also still works
v4(undefined, new Uint8Array(16))

[broofa]

I'd like to have a unit test for this. Can you add the following file:

---

examples/typescript/buffer.test.ts

import { v1 } from 'uuid';

// eslint-disable-next-line no-unused-vars
const ret: Buffer = v1(null, Buffer.alloc(16));

With that file in place, npm run examples:node:typescript:test should fail on main with this error:

buffer.test.ts:4:7 - error TS2740: Type 'Uint8Array' is missing the following properties from type 'Buffer': write, toJSON, equals, compare, and 66 more.

If I understand your use case correctly, this error should go away inyour branch, right?

[BurningEnlightenment]

If I understand your use case correctly, this error should go away inyour branch, right?

That's correct. I went ahead and added a buffer accepting overload to parse() similar to the v1-v7 overloads. Is that okay to include here?

EDIT: If the design is acceptable, I'd continue and adapt the documentation.

[broofa]

Please revert the API changes to parse(). I'm not averse to considering them, but they're out of scope for this PR. (Requires additional unit tests and assessing performance impact).

Without the parse() changes is there a need to update the docs?

[BurningEnlightenment]

Please revert the API changes to parse(). I'm not averse to considering them, but they're out of scope for this PR.

done

Without the parse() changes is there a need to update the docs?

Just a clarification in the uuid@11 TL;DR paragraph: 104713f

[broofa]

Published as uuid@11.1.0. Thanks for the contribution!

[BurningEnlightenment]

My pleasure!