Skip to content

fix: Build Uint8Array from ReadableStream without experimental Response constructor #1121

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

fix: Build Uint8Array from ReadableStream without experimental Response constructor #1121

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions packages/stream-collector-browser/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,20 @@
import { StreamCollector } from "@aws-sdk/types";

export const streamCollector: StreamCollector = (
stream: ReadableStream
export const streamCollector: StreamCollector = async (
stream: ReadableStream<Uint8Array>
): Promise<Uint8Array> => {
return new Response(stream)
.arrayBuffer()
.then(arrayBuffer => new Uint8Array(arrayBuffer));
let res = new Uint8Array(0);
const reader = stream.getReader();
let isDone = false;
while(!isDone) {
const { done, value } = await reader.read();
if(value) {
const prior = res;
res = new Uint8Array(prior.length + value.length);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@russell-dot-js russell-dot-js Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));
};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

Copy link
Contributor Author

@russell-dot-js russell-dot-js Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

res.set(prior);
res.set(value, prior.length);
}
isDone = done;
}
return res;
};