feat(response): added support to Uint8Array #540
Conversation
__tests__/integration.js
Outdated
| res.set('X-Custom-Header', singleNormal) | ||
|
|
||
| const readStream = new stream.PassThrough() | ||
| const buffer = Uint8Array.from( |
There was a problem hiding this comment.
You wrote in this PR’s description:
I added a test, but in the coverage I didn't see the new validation being marked as tested, any thoughts?
In your __tests__/integration.js file, Buffer.isBuffer(buffer) returns false,
and buffer instanceof Uint8Array returns true, both as expected.
However, when I print Buffer.isBuffer(buffer) and buffer instanceof Uint8Array in addData, both return true 😲
data is passed to addData in write: (data, encoding, cb) => {, but I don't know how where and why data suddenly makes Buffer.isBuffer return true.
There was a problem hiding this comment.
nodejs/node#14118 (comment) might help.
There was a problem hiding this comment.
I've seen this behavior of buffer instanceof Uint8Array covering cases of any buffer happening in my library too, Webstorm also tells me to remove data instanceof Uint8Array and leave just Buffer.isBuffer(data) but my tests failed when I passed Uint8Array, so you need to check this instanceof.
Well, maybe it's the internal behavior of express that makes this new validation not marked as tested, should we leave it as is or try to mark it in the test?
There was a problem hiding this comment.
Yeah, it looks like this issue comes from special behaviors of express and Next.js.
The original issue I posted mentioned Next.js (and the issue also happens in our Next.js apps), so if we wanted to have this particular case covered by tests, we would need to somehow replace express by Next.js (or some other app), which does not convert Uint8Array to Buffer.
So the test you added makes sense if the if (Buffer.isBuffer(data) case has not been tested elsewhere.
If the added test is only for covering || data instanceof Uint8Array, I personally would remove it
There was a problem hiding this comment.
When I reverse the validation to check the Uint8Array first, the validation is marked as tested:
But if I remove my test, the validation will still be marked as tested, so I'll remove my test because it's not actually testing anything.
Let's wait for Brett to see if he has any ideas on how to properly test this line.
# [4.10.0](v4.9.0...v4.10.0) (2022-07-25) ### Features * **response:** added support to Uint8Array ([#540](#540)) ([8b76331](8b76331))
|
🎉 This PR is included in version 4.10.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# [4.10.0](CodeGenieApp/serverless-express@v4.9.0...v4.10.0) (2022-07-25) ### Features * **response:** added support to Uint8Array ([#540](CodeGenieApp/serverless-express#540)) ([8b76331](CodeGenieApp/serverless-express@8b76331))
Issue #, if available:
#539
Description of changes:
Added support for checking if the data is Uint8Array because Next.js returns it in ssr.
About implementation
@brett-vendia I tried to add test to this new feature but I couldn't find anywhere to put it or how to test properly,
I added a test, but in the coverage I didn't see the new validation being marked as tested, any thoughts?
I also added support inside the
getStringfunction, in this dougmoscrop/serverless-http#232 the author didn't add it, but to me it makes sense that the function also has support for Uint8Array.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.