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

Update hapijs/shot to 1.5.1 from 1.5.0 #2586

Closed
hueniverse opened this issue Jun 4, 2015 · 5 comments
Closed

Update hapijs/shot to 1.5.1 from 1.5.0 #2586

hueniverse opened this issue Jun 4, 2015 · 5 comments

Comments

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Jun 4, 2015

No description provided.

@hueniverse hueniverse self-assigned this Jun 4, 2015
@hueniverse hueniverse added this to the 8.6.1 milestone Jun 4, 2015
@hueniverse
Copy link
Contributor Author

@hueniverse hueniverse commented Jun 4, 2015

@iamdoron @Marsup the changes in shot 1.5.1 break a bunch of the transmit tests. Before I dig deeper into this, is the problem with the hapi tests or the changes in shot?

@iamdoron
Copy link
Contributor

@iamdoron iamdoron commented Jun 4, 2015

Shot now stores the buffer in payloadBuffer. That should solve the first issue:

it('returns an JSONP response with compression', function (done) {

            var handler = function (request, reply) {

                var parts = request.params.name.split('/');
                return reply({ first: parts[0], last: parts[1] });
            };

            var server = new Hapi.Server();
            server.connection();
            server.route({
                method: 'GET',
                path: '/user/{name*2}',
                config: {
                    handler: handler,
                    jsonp: 'callback'
                }
            });

            server.inject({ url: '/user/1/2?callback=docall', headers: { 'accept-encoding': 'gzip' } }, function (res) {

                expect(res.headers['content-type']).to.equal('text/javascript; charset=utf-8');
                expect(res.headers['content-encoding']).to.equal('gzip');
                expect(res.headers.vary).to.equal('accept-encoding');
                Zlib.unzip(res.payloadBuffer, function (err, result) {
                    console.log(err);
                    expect(err).to.not.exist();
                    expect(result.toString()).to.equal('/**/docall({"first":"1","last":"2"});');
                    done();
                });
            });
        });

I'll look at the others

@iamdoron
Copy link
Contributor

@iamdoron iamdoron commented Jun 4, 2015

oh, I think I'm wrong - maybe there is an issue with the first one. I'll further look into it.

@iamdoron
Copy link
Contributor

@iamdoron iamdoron commented Jun 4, 2015

ok

So- the question is what is res.payload - is it a buffer? is it string in utf8? binary?
The thing is, when it was a 'binary' string, there was no problem with regular english strings and real binary strings (from a binary file, for instance), as they were in an equal encoding.

new Buffer("doron", 'utf8').toString('binary') === "doron"
new Buffer("\x89PNG\r", 'binary').toString('utf8') !== "\x89PNG\r"

After the change I introduced in shot, now res.payload is utf8, res.payloadBuffer is Buffer. I suggest to change res.payload to buffer, and let either hapi or the user decide what to do with it, since the buffer represents accurately the data being transmitted - maybe hapi tries to parse it to JSON or leave it be.

@iamdoron
Copy link
Contributor

@iamdoron iamdoron commented Jun 4, 2015

There was a problem with the names - hapijs/shot#32

according to hapi api docs:

  • payload - the response payload string. (UTF8?)
  • rawPayload - the raw response payload buffer.

@hueniverse hueniverse closed this in 6002768 Jun 5, 2015
@hueniverse hueniverse added the bug label Jun 5, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Jan 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants