fix: remove Blob usage

[blakeembrey]

Issue #, if available: Closes #1378.

Description of changes: Removes Blob constructor usage from code paths where it's not necessary. I also found a past perf test when I was looking for alternative solutions to getting the string length and Blob is at least 130x slower than a for loop. Using TextEncoder would be even faster, but I'm assuming we want IE11 support natively without any polyfills.

Perf test: https://jsperf.com/utf-8-byte-length/36
Other possible solutions: https://stackoverflow.com/questions/5515869/string-length-in-bytes-in-javascript

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[codecov-commenter]

Codecov Report

Merging #1384 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1384   +/-   ##
=======================================
  Coverage   79.07%   79.08%           
=======================================
  Files         287      287           
  Lines       11150    11155    +5     
  Branches     2358     2360    +2     
=======================================
+ Hits         8817     8822    +5     
  Misses       2333     2333           

Impacted Files	Coverage Δ
packages/util-body-length-browser/src/index.ts	83.33% <0.00%> (+11.90%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5323a81...1b68073. Read the comment docs.

[blakeembrey]

If I understand the purpose of this code correctly, this could even be a feature check on response.body instead so collectBlob or collectStream would only run when the response follows the same logic in

aws-sdk-js-v3/packages/fetch-http-handler/src/fetch-http-handler.ts

Line 74 in 5323a81

return response.blob().then((body) => ({

I believe this is only for IE support too, so if you’re dropping IE this would all be unnecessary.

[blakeembrey]

Actually, now that I think about this IE doesn’t even support fetch. Is there a specific reason for using blob()?

[blakeembrey]

Potential feature check that would cover both:

'body' in Response.prototype

This would allow all non-IE browsers to run using the optimized stream code path while IE runs in the blob path, and then there's no need for checking stream instanceof Blob anymore.

[AllanZhengYP]

To answer your question:

Actually, now that I think about this IE doesn’t even support fetch. Is there a specific reason for using blob()?

This is necessary for old browsers without Stream API support, and React-Native.

This code would look nicer if

export const streamCollector: StreamCollector = (stream: Blob | ReadableStream): Promise<Uint8Array> => {
  if (typeof Blob === "function" &&  stream instanceof Blob) {
    return collectBlob(stream);
  }
  return collectStream(stream);
}

[blakeembrey]

Wouldn't 'body' in Response.prototype handle this? If it does, you could do away with the runtime check altogether. I don't have an old browser or React Native set up to test, but reading over the fetch API by GitHub it would return false while modern browsers return true.

[blakeembrey]

Of course, happy to do it in the runtime code path if you do prefer, it's not a big difference.

[blakeembrey]

Also, using typeof Blob === "function" && stream instanceof Blob won't pass the type checker without type assertions. Is that ok?

[AllanZhengYP]

Regarding using 'body' in Response.prototype in the Fetch HTTP handler, I prefer to have the feature check in stream collector too. Because stream collector and Fetch http handler can be exported separately, I don't like them to be intertwined too much.

[AllanZhengYP]

Thanks for this PR, and thank you for pointing out the performance difference of using Blob to get the string byte length. I just have a few comments.

[AllanZhengYP]

To answer your question:

Actually, now that I think about this IE doesn’t even support fetch. Is there a specific reason for using blob()?

This is necessary for old browsers without Stream API support, and React-Native.

This code would look nicer if

export const streamCollector: StreamCollector = (stream: Blob | ReadableStream): Promise<Uint8Array> => {
  if (typeof Blob === "function" &&  stream instanceof Blob) {
    return collectBlob(stream);
  }
  return collectStream(stream);
}

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.