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

Add a note about gzip being incompatible with HMR #10

Closed
STRML opened this issue Sep 8, 2015 · 12 comments
Closed

Add a note about gzip being incompatible with HMR #10

STRML opened this issue Sep 8, 2015 · 12 comments

Comments

@STRML
Copy link

STRML commented Sep 8, 2015

If webpack-hot-middleware is installed after gzip middleware (like compression), no events will be sent as the buffers are not properly flushed.

This took a while to track down so it would be great if it were documented.

Alternatively, we can check for the existence of a res.flush() method and call it. This would make SSE work even if gzip is used. See expressjs/compression#17

@glenjamin
Copy link
Collaborator

Hrm, this is annoying. I can add in some notes to say that SSE is involved.

I'm not massively keen on implementing a non standard stream method. Does the gzip middleware return true/false to indicate buffering status on write()? Could pump extra bytes in until it indicates it was completed - or maybe get it to treat write("") as a flush?

@glenjamin
Copy link
Collaborator

Perhaps https://github.com/jshttp/compressible shouldn't attempt to compress text/event-stream?

@STRML
Copy link
Author

STRML commented Sep 8, 2015

I'm looking for a way to trick the compressor although I doubt there is a foolproof way.

There's nothing wrong with compressing text/event-stream, except that you need to manually flush after each write.

I agree that flush seems like a hack but I'm not sure if there is a better choice or that it would be any less robust than munging headers to trick compression.

@glenjamin
Copy link
Collaborator

Perhaps @dougwilson could weigh in? How best to not break upstream buffering plugins without specifically coding for compression?

@dougwilson
Copy link

All we do is pump to the built-in zlib.createGzip in Node.js. Their stream is what requires the .flush() calls, which is why we expose it on res. I don't think there's any way around it. We also do properly return true/false on writes (it's just whatever value the .write() on the Node.js gzip stream returned).

@dougwilson
Copy link

One possibility is that we could, perhaps, change the default response filter to look for a Cache-Control response header, and not compress if the declaration no-transform is in there, which could be a good general way you could opt-out of compression without requiring users to do a bunch of configuration and that, as a generic default, makes sense to me. Thoughts?

@STRML
Copy link
Author

STRML commented Sep 8, 2015

That seems good to me and fits the semantics of no-transform.

@dougwilson
Copy link

Cool. Tracking issue: expressjs/compression#51

@glenjamin
Copy link
Collaborator

I can pick up doing a PR on both sides of no-one else beats me to it

@marcello3d
Copy link

Also spent a couple hours tracking down this bug (twice, in fact, several weeks apart). :-)

@glenjamin
Copy link
Collaborator

Added a troubleshooting note in 129de82, the new header is included in v2.0.2, but still waiting on the upstream PR to be merged into compression.

Will leave this bug open until that's merged too.

@dougwilson
Copy link

Published to npm as compression@1.6.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants