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

ENHANCEMENT - don't compress responses with the x-no-compression response header #7232

Merged
merged 2 commits into from Sep 14, 2017
Merged

ENHANCEMENT - don't compress responses with the x-no-compression response header #7232

merged 2 commits into from Sep 14, 2017

Conversation

akatov
Copy link
Contributor

@akatov akatov commented Jul 16, 2017

I'm using an http-proxy to proxy to an endpoint intended for long polling. Now this endpoint writes a newline to the HTTP connection in regular intervals to make sure the connection stays alive.

The compression middleware however buffers the output produced by the endpoint (and the http-proxy) waiting for it to complete so it can compress it. This leads to the initiator closing the connection after not receiving back any data.

Having the proposed change in place lets me circumvent this problem by setting a header on the response object like this:

// server/proxies/long-poll.js
module.exports = function(app) {
  app.use('/long-poll', function(req, res) {
    res.setHeader('x-no-compression', 'true');
    require('http-proxy').createProxyServer().web(req, res, {
      target: 'http://long-polling.service.lvh.me:1234'
    });
  });
};

@stefanpenner
Copy link
Contributor

I was going to say, maybe expressjs/compression supports this already, but apparently there readme suggests your approach https://github.com/expressjs/compression#filter-1!

I am fine with this change, but it likely needs a unit test. Or it may regress overtime.

@stefanpenner
Copy link
Contributor

Also looks like your tests are failing due to an eslint issue:

not ok 10 eslint should have no errors in lib
  Error: Code did not pass lint rules
  lib/tasks/server/express-server.js
    204:8  error  Missing trailing comma  comma-dangle
  
  ✖ 1 problem (1 error, 0 warnings)
  
      at Context.<anonymous> (node_modules/mocha-eslint/index.js:36:15)

locally, you can run npm run lint to check linting rules without having to run the full tests.

@akatov
Copy link
Contributor Author

akatov commented Jul 20, 2017

I fixed the eslint issue.
I'd also be grateful if you could point me to most fitting existing unit test to use as a blueprint for this feature?

@akatov
Copy link
Contributor Author

akatov commented Aug 27, 2017

OK, added a unit test. Not really sure how to check whether the actual body is compressed, so I'm just checking for the correct response headers...

@akatov
Copy link
Contributor Author

akatov commented Sep 11, 2017

rebased on v2.15.1

@rwjblue
Copy link
Member

rwjblue commented Sep 14, 2017

Thanks for working on this!

@akatov akatov deleted the compression-filter branch October 30, 2017 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants