fix: Build Uint8Array from ReadableStream without experimental Response constructor

[russell-dot-js]

Issue #, if available:
#1107

Description of changes:
Build Uint8Array from ReadableStream without experimental Response constructor

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

[codecov-io]

Codecov Report

Merging #1121 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1121   +/-   ##
=======================================
  Coverage   73.54%   73.54%           
=======================================
  Files         283      283           
  Lines       13014    13014           
  Branches     3030     3030           
=======================================
  Hits         9571     9571           
  Misses       3443     3443           

---

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 dae6d08...1303244. Read the comment docs.

[aws-sdk-js-automation]

AWS CodeBuild CI Report

• CodeBuild project: sdk-staging-test
• Commit ID: 1303244
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[AllanZhengYP]

Looks good, only have one question.

[AllanZhengYP]

Instead of assigning the typed array multiple times, can we push the chunks into an Array<Uint8Array> and only keep track of the total length here? When the stream is done, we can create the concatenated buffer one time and assign those chunks value.

[russell-dot-js]

yes, we can. I played around with that implementation at first. either is fine, but my (limited) understanding of TypedArray is that they are using the same underlying memory so this line is much more "free" than allocating new arrays and pushing into them.

Essentially we are weighing the computational cost of n new Uint8Array vs the storage cost of Uint8Array[n]. They should both theoretically be cheap.

LMK if you have a strong preference for the Uint8Array[], I'll get right on it

[AllanZhengYP]

Alright, it looks good to me! I don't have strong preference either, I think the current implementation is cleaner.
⛴

[russell-dot-js]

FWIW, here's the other implementation:

export const streamCollector: StreamCollector = async (
  stream: ReadableStream<Uint8Array>
): Promise<Uint8Array> => {
  const chunks: Uint8Array[] = [];
  const reader = stream.getReader();
  let isDone = false;
  while(!isDone) {
    const { done, value } = await reader.read();
    if(value) {
      chunks.push(value);
    }
    isDone = done;
  }
  return chunks.reduce((acc: Uint8Array, chunk: Uint8Array) => {
    const next = new Uint8Array(acc.length + chunk.length);
    next.set(acc);
    next.set(chunk, acc.length);
    return next;
  }, new Uint8Array(0));
};

[russell-dot-js]

FWIW, here's the other implementation:

export const streamCollector: StreamCollector = async (
  stream: ReadableStream<Uint8Array>
): Promise<Uint8Array> => {
  const chunks: Uint8Array[] = [];
  const reader = stream.getReader();
  let isDone = false;
  while(!isDone) {
    const { done, value } = await reader.read();
    if(value) {
      chunks.push(value);
    }
    isDone = done;
  }
  return chunks.reduce((acc: Uint8Array, chunk: Uint8Array) => {
    const next = new Uint8Array(acc.length + chunk.length);
    next.set(acc);
    next.set(chunk, acc.length);
    return next;
  }, new Uint8Array(0));
};

In hindsight, .reduce does exactly what the current implementation does, and has no benefits. I think you were asking for a standard for loop (e.g. const res = new Uint8Array(combinedLengthOfAllChunks))

[russell-dot-js]

Here you go:

  const chunks: Uint8Array[] = [];
  const reader = stream.getReader();
  let isDone = false;
  while(!isDone) {
    const { done, value } = await reader.read();
    if(value) {
      chunks.push(value);
    }
    isDone = done;
  }

  const totalSize = chunks
    .map(c => c.length)
    .reduce((l1, l2) => l1 + l2, 0);

  const res = new Uint8Array(totalSize);
  let offset = 0;
  chunks.forEach(c => {
    res.set(c, offset);
    offset += c.length;
  });
  return res;

I agree that current is cleaner. But LMK what you prefer.

[russell-dot-js]

I am testing the changes on a clean branch, and I am getting a new error in IE11. I'm guessing I had some other polyfill uncommitted that I since wiped. I'm going to at least figure out what's needed for this to work in IE11 before asking for a merge and letting the amplify team know it's fixed.

[russell-dot-js]

I am testing the changes on a clean branch, and I am getting a new error in IE11. I'm guessing I had some other polyfill uncommitted that I since wiped. I'm going to at least figure out what's needed for this to work in IE11 before asking for a merge and letting the amplify team know it's fixed.

Ok, I finished digging in and found a production-ready solution (detailed here). This can be merged as it has no negative consequences, but does not fix every issue. Would love to hear your thoughts and help in any way I can.

[AllanZhengYP]

@russell-dot-js Your suggestion regarding pollyfilling makes perfect sense to me. So I plan to get my hands on reproducing the issue in order to gain a higher level picture on the SDK support in IE/Edge. I will hold mergin this PR for a day or two before I finishing my homework.

[Stereobit]

@AllanFly120 Did you have time to look at this yet? I have a significant amount of users who can't log into my application as the amplify package uses the AWS SDK beta. Is there anything I can help with?

[AllanZhengYP]

Close this PR as it's duplicated with #1123-diff. We will progress with the other PR. Let me know when you kneed to re-open this one.

[lock]

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.