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

unzip does not work when using pipe #240

Merged
merged 2 commits into from
Jul 5, 2013
Merged

Conversation

timoxley
Copy link
Contributor

@timoxley timoxley commented Jul 3, 2013

Related to componentjs/component#369

simple fix, there's a lot of stuff going on in utils.unzip so I just skipped that and made it work.

But, this is total crap because it unzips the thing twice because it still runs https://github.com/timoxley/superagent/blob/e36814a3e5a9b95fc68fb7a76bf2feb79a5772ee/lib/node/index.js#L679-L682

// zlib support
if (/^(deflate|gzip)$/.test(res.headers['content-encoding'])) {
  utils.unzip(req, res);
}

Which, I guess, isn't any more inefficient than it the current code, since using .pipe also unzips the data but it just does nothing with it, haha great.

This makes it work, but yeah sucks, and overall that area's code could do with straightening out.

@brighthas
Copy link

very good ! 👍

@timoxley
Copy link
Contributor Author

timoxley commented Jul 3, 2013

ugh, this breaks on node 0.8. Investigating.

@timoxley
Copy link
Contributor Author

timoxley commented Jul 3, 2013

@visionmedia help :(

@tj
Copy link
Contributor

tj commented Jul 3, 2013

i get a different error in each version of node :( checking it out

@tj
Copy link
Contributor

tj commented Jul 3, 2013

I almost think we should just get rid if .pipe() it's pretty horrid trying to get things to work properly

@tj
Copy link
Contributor

tj commented Jul 3, 2013

formidable is super unhappy too for whatever reason, used to work fine :s might have to revert to an older version

@tj
Copy link
Contributor

tj commented Jul 3, 2013

just disabling multipart for now it's so broken, i'll try and check the rest of this out soon

@timoxley
Copy link
Contributor Author

timoxley commented Jul 4, 2013

ah, yeah, multipart is borked, even without my changes. I have a feeling, at least in theory, a more streams-oriented approach will actually help straighten the code out here, but you're likely to disagree with that haha

@tj
Copy link
Contributor

tj commented Jul 4, 2013

yeah haha I stay as far away from them as possible, especially after seeing how broken the streams 2 implementation is, we'd be rewriting things left and right. I haven't had tons of time to look into it but it seems like it may be a node bug: node-formidable/formidable#240

really hard to follow core's code though, but the removal of agent and keep-alive is what screws over all the multipart tests, which is kind of interesting because that means there is probably a race regardless, just one that is intermittent when agent is enabled.

tj added a commit that referenced this pull request Jul 5, 2013
unzip does not work when using pipe
@tj tj merged commit 6306c12 into ladjs:master Jul 5, 2013
@tj
Copy link
Contributor

tj commented Jul 5, 2013

im getting


  1) zlib with pipe should deflate during pipe:
     Uncaught Error: incorrect header check
      at Zlib._binding.onerror (zlib.js:286:17)

@tj
Copy link
Contributor

tj commented Jul 5, 2013

oh nvm you said it breaks 0.8.x

@tj
Copy link
Contributor

tj commented Jul 5, 2013

definitely need to rework this lib, even when we get this working .pipe() is still super half-assed, won't follow redirects or anything

tj added a commit that referenced this pull request Jul 5, 2013
@tj
Copy link
Contributor

tj commented Jul 5, 2013

fixed that but now one of the stream tests fails :S haha.. woot

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