fix: use Uint8Array instead of Buffer

[Uzlopak]

We should avoid the use of Buffer and instead use Uint8Array.

[github-actions]

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

[gr2m]

We should avoid the use of Buffer and instead use Uint8Array.

I agree.

But does this introduce a breaking change to the end user?

[Uzlopak]

No, the unit test was wrong in the first place, because it passes a string, despite that the streams should use Buffers/Uint8Arrays anyway. It was just working, because somewhere the string got transformed to Buffer. If I would have written that unit test long time ago correct, it would not triggered that failing unit test today. :D

[Uzlopak]

It was working, because of

webhooks.js/src/middleware/node/get-payload.ts

Line 38 in 9a9593a

? data[0].toString("utf8")

So the string "bar" would be then "bar".toString()

I guess in the moment where we emit two times strings, it would error because Buffer.concat expects only Uint8Array and Buffers.

[gr2m]

Okay I say we ship it, thanks for all the great work @Uzlopak!

But I suggest we do a release even though this is technically a refactor as far as I can tell. Or we do a feature release saying it introduces compatibility with Uint8Array? I'm not sure what the better option is, but I would like us to do a release just in case the changes cause problems in some obscure JS runtime environment, it will make it easier to track down the problem and to revert the change if necessary.

[Uzlopak]

@gr2m

You mean this change as last minor of the current major?

[Uzlopak]

@gr2m

Sure, makes sense.

I wrote following test case in main branch.

  it("resolves with a string if the body key of the request is defined but value is undefined", async () => {
    const request = new EventEmitter();
    // @ts-ignore body is not part of EventEmitter, which we are using
    // to mock the request object
    request.body = undefined;

    const promise = getPayload(request);

    // we emit data, to ensure that the body attribute is preferred
    request.emit("data", "foo");
    request.emit("data", "bar");
    request.emit("end");

    expect(await promise).toEqual("foobar");
  });

And I got following result:

 FAIL  test/integration/get-payload.test.ts > getPayload > resolves with a string if the body key of the request is defined but value is undefined
TypeError: The "list[0]" argument must be an instance of Buffer or Uint8Array. Received type string ('foo')
 ❯ EventEmitter.<anonymous> src/middleware/node/get-payload.ts:39:20
     37|         data.length === 1
     38|           ? data[0].toString("utf8")
     39|           : Buffer.concat(data).toString("utf8"),
       |                    ^
     40|       ),
     41|     );
 ❯ test/integration/get-payload.test.ts:105:13

So definetly the test was wrong by passing a string.

[github-actions]

🎉 This PR is included in version 13.8.3 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀