Skip to content
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.

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

Use async iterators to get response body #1256

Merged
merged 6 commits into from
Jul 3, 2020

Conversation

szmarczak
Copy link
Collaborator

Just to utilize newest Node.js features :)

Checklist

  • I have read the documentation.
  • I have included a pull request description of my changes.
  • I have included some tests.
  • If it's a new feature, I have included documentation updates.

@sindresorhus
Copy link
Owner

Why not just do this directly in get-stream? We're planning to do the change there anyway.

@szmarczak
Copy link
Collaborator Author

Why not just do this directly in get-stream? We're planning to do the change there anyway.

Sure, I just want to test things before we make an actual release :)

@szmarczak
Copy link
Collaborator Author

I'll merge this for now. I think this may fix #1187.

@szmarczak szmarczak merged commit 7dcd145 into sindresorhus:master Jul 3, 2020
@szmarczak szmarczak deleted the get-buffer branch July 3, 2020 11:34
szmarczak added a commit to jaulz/got that referenced this pull request Jul 6, 2020
@arcanis
Copy link

arcanis commented Oct 10, 2020

@szmarczak We upgraded got in Yarn, and I noticed it caused a breakage with all requests being returned with no content (it doesn't happen to other contributors, weirdly, even on the same Node versions; I'm on OSX, but our CI tests are passing there too). After investigating, the problem I experience appeared between the 11.3 and the 11.4 releases, and more specifically by this PR.

I can patch it locally by replacing the following:

for await (const chunk of stream) {
    chunks.push(chunk);
    length += Buffer.byteLength(chunk);
}

By this:

const {PassThrough} = require(`stream`);

const copy = new PassThrough();
stream.pipe(copy);

for await (const chunk of copy) {
    chunks.push(chunk);
    length += Buffer.byteLength(chunk);
}

I suspect this is because the various Node stream interfaces aren't meant to be used together, and weird effects are possible should that happen. I don't know if that's the case for sure here 🙁

@szmarczak
Copy link
Collaborator Author

@arcanis That seems weird. What Node.js version are you using? It'd be best to open a new issue so we don't lose the track here.

This looks like a Node.js bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants