Fix proxy for chunked streaming templates

[sfcgeorge]

Fixes #2195

Webpacker is currently incompatible with Rails template streaming (Transfer-Encoding: chunked) in development. Firstly it prevents streaming; the whole layout blocks. Secondly it inserts junk into the page that breaks rendering in browsers.

This patch fixes it. I'm unclear on why we even need to proxy everything, or why the proxy is messing with headers. Nonetheless, this line deletes the header that is needed for streaming to work, which this patch amends. It also fixes the junk in the page, I assume because deleting that header when the server is streaming leads to an invalid format.

[jakeNiemiec]

This does not account for: Transfer-Encoding: gzip, chunked

I would just remove that line, it is from: #637

[sfcgeorge]

Thanks for the review. Ooh good point, so 3 options:

1. Delete the line (I'm hesitant as it must've been added for a reason so I've asked Add dev server config class and setup proxy #637 (comment)).
2. Add the gzip, chunked case as well.
3. Leave it; chunking is done after gzipping which implies this case won't happen with Rails because Rails streaming happens first so you can't gzip afterwards. I can confirm the the Rack::Deflate middleware breaks streaming. That said, someone could potentially have another middleware that does chunking later for whatever reason.

I'm also in favour of 1 for simplicity if the line turns out to not be needed so I'll see if I get a response then go from there :)

[jakeNiemiec]

I say delete because:

1. 2 years ago, webpacker was a different tool with different goals
2. This prevents any file size compression
3. transfer-encoding does not seem to even be present to delete. I tested the major webpacker flows and this seems to be nonessential.

[sfcgeorge]

Thanks for the testing! Awesome I'll go with 3 then.

In my testing the header wasn't deleted but the page was broken. I assume something else was adding it back in again, possibly this middleware that adds chunked if there's no content-length set.

[sfcgeorge]

What do you think about the line below deleting the content-length header? One or the other is required by HTTP 1.1, so I thought maybe this middleware was relying on the Rack::Chunked middleware to add back Transfer-Encoding: chunked. But running bin/rails middleware I don't see Rack::Chunked in the stack, yet I always seem to have that Transfer-Encoding: chunked header as you said.

[jakeNiemiec]

I assume something else was adding it back in again

Weirdly enough, chromes devtool adds it to the request for me. curl http://localhost:3000/streaming -I shows it missing.

What do you think about the line below deleting the content-length header?

I honestly have no idea; I am not that much of an expert on Rack/middleware stuff.

[gauravtiwari]

Thanks, both and apologies for the delay. Let's get rid of that line if it works now. Earlier, there was some weird error from dev server (like invalid chunk) or something. Basically, the packs arrived broken in browser.

I can't exactly remember but if it works now let's remove it.

[sfcgeorge]

Thank you @gauravtiwari 😄

Okay I've removed rewrite_response now. I was nervous about simply removing but:

• Without knowing what the purpose was and without a test I agree it shouldn't blindly stay.
• This fixes the chunked encoding bug.
• This middleware isn't inserted in production (usually) so previously there would've been a difference in headers between development and production envs that could cause other hard to debug issues.
• The old behaviour modified the headers of all requests, not just asset requests which is overkill.

That said, if after merging someone does have an issue due to an unusual WDS config then we can add a test case and put the lines back; but wrapped in a check so only assets have their headers modified:

if env["PATH_INFO"].start_with?("/#{public_output_uri_path}") && dev_server.running?
  headers.delete "transfer-encoding"
  headers.delete "content-length" if dev_server.running? && dev_server.https?
end

[jakeNiemiec]

I'm unclear on why we even need to proxy everything, or why the proxy is messing with headers.

Webpack dev server needs it. It's an express server that hosts assets and creates a websocket that your page connects to for hot reloading.

[sfcgeorge]

I see from the PR you referenced that the proxy is so assets are served from the same host to prevent CORS issues, makes sense :)

[gauravtiwari]

Thanks @sfcgeorge @jakeNiemiec

Let give this a try and will cut a new release if no issues arises.