Add a note about gzip being incompatible with HMR
STRML opened this issue · 12 comments
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
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?
Perhaps https://github.com/jshttp/compressible shouldn't attempt to compress text/event-stream?
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.
Perhaps @dougwilson could weigh in? How best to not break upstream buffering plugins without specifically coding for compression?
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).
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?
That seems good to me and fits the semantics of no-transform.
Cool. Tracking issue: expressjs/compression#51
I can pick up doing a PR on both sides of no-one else beats me to it
Also spent a couple hours tracking down this bug (twice, in fact, several weeks apart). :-)
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.
Published to npm as compression@1.6.0