Skip to content

Conversation

@tmoitie
Copy link

@tmoitie tmoitie commented Jun 15, 2016

When I ran the test suite I got nothing from length, because the default length was 0 and the early return from the pipe listener was if (typeof length === 'number') return;

I've also updated all deps to their latest versions.

@freeall
Copy link
Owner

freeall commented Jun 16, 2016

Super that you updated the dependencies. But I'm not sure it makes sense that a default value for length is null. Can you explain a little more why it should be a good idea?

@tmoitie
Copy link
Author

tmoitie commented Jun 16, 2016

Because length is set in the pipe event, and if it defaults to 0 it always gets caught in the first condition.

It might be that I went around this the wrong way and the bug is actually that first condition.

tr.on('pipe', function(stream) {
        if (typeof length === 'number') return;
        // Support http module
        if (stream.readable && !stream.writable && stream.headers) {
            return onlength(parseInt(stream.headers['content-length'] || 0));
        }

        // Support streams with a length property
        if (typeof stream.length === 'number') {
            return onlength(stream.length);
        }

        // Support request module
        stream.on('response', function(res) {
            if (!res || !res.headers) return;
            if (res.headers['content-encoding'] === 'gzip') return;
            if (res.headers['content-length']) {
                return onlength(parseInt(res.headers['content-length']));
            }
        });
    });

@freeall
Copy link
Owner

freeall commented Jun 16, 2016

I'm not sure what you're doing, but couldn't you check if length === 0?

@tmoitie
Copy link
Author

tmoitie commented Jun 16, 2016

I don't know the module well enough to answer that. It would also fix the bug if you did if (length !== 0) return;, but I'm not sure on the ramifications of that in terms of if a user wanted to legitimately set length to 0.

@freeall
Copy link
Owner

freeall commented Jun 16, 2016

So I can't exactly remember why we have the number check. It was added in this commit a94474f. Maybe @mafintosh can remember why it was added?

@freeall
Copy link
Owner

freeall commented Jun 16, 2016

@tmoitie Do you have an example where this doesn't work as expected?

@tmoitie
Copy link
Author

tmoitie commented Jun 16, 2016

The test suite in the package is a good start. The commenter on that commit even highlighted the problem

example

@mafintosh
Copy link
Contributor

@freeall yep that check should do a truthiness check on opts.length as well

@tmoitie
Copy link
Author

tmoitie commented Jun 16, 2016

Like that @mafintosh?

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