[feature] send blob response

[debadutta98]

Hi @dougwilson and Team

I hope this improves the feature of express 🚀🚀

This PR is related to #4807

• Send blob response using res.send(blob)
• Added testcases
• It only accept the instance of node Blob Object

Thank you 😊

[debadutta98]

Hi @dougwilson and @abenhamdine just made some changes please verify

[debadutta98]

hi @dougwilson , is this PR need any further improvements or is anything still pending from the end to complete this ?

[debadutta98]

hi @dougwilson, as you can see that all the test cases have been passed so can we merge this now?

[debadutta98]

Hi @dougwilson, I think all issues reported by you are resolved. hope to merge this soon.

Thank you!!

[debadutta98]

Hi @dougwilson, can you please re-run the GitHub action?
There might be an issue with the test run.

Thank you !!

[debadutta98]

@dougwilson is there any issues which is still there in this PR?

[debadutta98]

@dougwilson just to remind you about this.

[krzysdz]

I see that your code populates Content-Length in the switch/case where the type is set for all types of arguments and you skip the populate Content-Length blocks. This means that ETag will never be set. I assume that this is on purpose, because the etag package does not work with Blobs and Files.

Maybe it would be worth considering adding support for Blob/File to etag first?
The problem is that the crypto.Hash API requires a string, Buffer, TypedArray or DataView and that would require calling blob.arrayBuffer(), which copies the data (and is asynchronous). For Files, however, a tag based on size and lastModified could be used like for regular files.

Also, your code formatting is inconsistent (sometimes lacking spaces between if and ( or ) and {; semicolons are used most of the time, but not always), but this is just nitpicking, especially since the existing code does not adhere to the JavaScript Standard Style (required by the Express Collaborator Guide) either.

[debadutta98]

I see that your code populates Content-Length in the switch/case where the type is set for all types of arguments and you skip the populate Content-Length blocks. This means that ETag will never be set. I assume that this is on purpose, because the etag package does not work with Blobs and Files.

Maybe it would be worth considering adding support for Blob/File to etag first? The problem is that the crypto.Hash API requires a string, Buffer, TypedArray or DataView and that would require calling blob.arrayBuffer(), which copies the data (and is asynchronous). For Files, however, a tag based on size and lastModified could be used like for regular files.

Also, your code formatting is inconsistent (sometimes lacking spaces between if and ( or ) and {; semicolons are used most of the time, but not always), but this is just nitpicking, especially since the existing code does not adhere to the JavaScript Standard Style (required by the Express Collaborator Guide) either.

Hi @krzysdz, I don't find any lint issue or something like that on this particular piece of code that I have written here by running npm run lint if find any please make your review comment on this I will check it out.

[debadutta98]

Hi @dougwilson @krzysdz and Team
Implementing etag mechanism for the cache control will take a lot of time to configure on this module. so can we need to raise a different issue of etag support for Blob and File streaming or should we make it happen on this PR? if we need to implement it on this PR can we discuss how we are going to achieve this before implementation?

[krzysdz]

Blob support for etag is definitely out-of-scope for this PR, since it is a separate package (jshttp/etag) and, as I already wrote, it may not be possible to do efficiently (or at all without significant changes to its API). While the lack of ETag for Blob is not a breaking change, since Blobs aren't supported yet in Express, it is something that others may found confusing, because ETag is generated for all other types of arguments. Ultimately, it's up for @dougwilson to decide, whether the omission of ETag for Blob is acceptable.

As for the code style, npm run lint executes eslint, which does not perform many checks (see .eslintrc.yml). The code style referred to in the the Express Collaborator Guide has many more rules which are not verified and not always followed in the existing code.

[debadutta98]

Hi @dougwilson, can you please give some time to this PR?

[debadutta98]

@dougwilson Ping

[debadutta98]

@UlisesGascon , Could you please review this pull request and merge it if everything looks good? If there are any areas that need improvement, could you let me know?

[wesleytodd]

This is a long thread (mostly with the OP spamming 😭, please stop spamming maintainers), AFAICT the comment with the most context is from @jimmywarting which seems to me to conclude that this feature is not really necessary for anything other than in node 14.

Unless anyone else can summarize it in a digestible way with a clear argument in favor of adding support for this I am a 👎 on landing this.