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

If no Accept-Encoding header is sent, koa-compress may compress the response #112

Closed
dobesv opened this issue May 14, 2020 · 9 comments
Closed

Comments

@dobesv
Copy link
Contributor

dobesv commented May 14, 2020

I found it very confusing when I used curl to hit my endpoint and got a compressed response dumped to my console. It seems like if curl doesn't send the Accept-Encoding header up it does not decompress the response if it gets a compressed response in return.

Nevertheless, I think a client that does not send Accept-Encoding should not be assumed to support gzip or any other compression scheme - by default they should not get any compressed responses.

@niftylettuce
Copy link
Contributor

niftylettuce commented Jul 4, 2020

I added a comment to #110 - once PR is updated or a new one made, I will merge and release as major semver bump.

@uhop
Copy link
Contributor

uhop commented Jul 5, 2020

Will do.

@uhop
Copy link
Contributor

uhop commented Jul 5, 2020

Done.

@niftylettuce
Copy link
Contributor

Merged. Once @jonathanong gives me npm access, I will publish v5.0.0 to npm and GitHub releases.

@niftylettuce
Copy link
Contributor

niftylettuce commented Jul 6, 2020

@uhop I just tested locally and I seem to have a failing test:

> [email protected] test /Users/test/Projects/compress
> jest

 PASS  __tests__/encodings.js
 PASS  __tests__/middleware.js
 FAIL  __tests__/index.js
  ● Compress › should not compress if no accept-encoding is sent

    assert(received)

    Expected value to be equal to:
      true
    Received:
      false

      263 |         if (err) { return done(err) }
      264 |
    > 265 |         assert(!res.headers['content-encoding'])
          |         ^
      266 |         assert(!res.headers['transfer-encoding'])
      267 |         assert.equal(res.headers['content-length'], '1024')
      268 |         assert.equal(res.headers.vary, 'Accept-Encoding')

      at Test.<anonymous> (__tests__/index.js:265:9)
      at Test.assert (node_modules/supertest/lib/test.js:181:6)
      at localAssert (node_modules/supertest/lib/test.js:131:12)
      at node_modules/supertest/lib/test.js:128:5
      at Test.Request.callback (node_modules/superagent/lib/node/index.js:716:12)
      at Stream.<anonymous> (node_modules/superagent/lib/node/index.js:916:18)
      at Unzip.<anonymous> (node_modules/superagent/lib/node/unzip.js:55:12)

@niftylettuce
Copy link
Contributor

The unset seems to not be working, hmm.

@niftylettuce
Copy link
Contributor

v5.0.0 published to npm and GitHub

https://github.com/koajs/compress/releases/tag/v5.0.0

@uhop
Copy link
Contributor

uhop commented Jul 6, 2020

Strange — I modified the test to not use unset()… Are you sure you tested the right thing?

@niftylettuce
Copy link
Contributor

I merged the wrong PR haha. You had two, #110 and #120, I merged #110 not #120. I reverted #110 and merged #120 and released to npm.

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

No branches or pull requests

3 participants