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

Content-Type:application/octet-stream being set for subsequent middleware #23

Closed
noinkling opened this issue Feb 12, 2017 · 2 comments · Fixed by #24
Closed

Content-Type:application/octet-stream being set for subsequent middleware #23

noinkling opened this issue Feb 12, 2017 · 2 comments · Fixed by #24

Comments

@noinkling
Copy link
Contributor

noinkling commented Feb 12, 2017

Already mentioned in #15 (and leecade/koa-webpack-middleware#23), but I thought I'd open a new issue.

Content-Type:application/octet-stream is being set for any middleware hit after this one. Besides being generally incorrect if you're trying to respond with html/text/json, it causes the browser (Chrome at least) to prompt for a download. Working around it means manually setting ctx.type to something more appropriate, or else making sure the webpack middleware only gets called on a route where it won't be a problem.

This behaviour is a symptom of context.body being unconditionally set to a stream (the passthrough) in the hot middleware part here. As soon as context.body gets assigned, Koa sets the appropriate content type, which then remains even after later reassigning body to a string or whatever.

Side note: writeHead should be setting context.status, not context.state, it should look like this:

writeHead: (status, headers) => {
  context.status = status;
  context.set(headers);
}

But the method that has been used to hook-in seems a bit hacky and brittle in general to me. I'll see if I can find the time to put together a PR, just wanted to raise this issue while it's in my head.

Thanks for creating this repo by the way, the other one isn't maintained very well 😛

@shellscape
Copy link
Owner

thanks for taking the time to triage this. I'll be sure to tag you in the PR if I get to it before you.

@noinkling
Copy link
Contributor Author

noinkling commented Feb 14, 2017

I opened a PR with a minimal fix, but obviously things remain quite brittle. While I was trying to think of a way to solve this I came up with a few other ideas:

  1. Duplicate the path check from the original middleware, only mess with the response and invoke the original middleware if it passes. Not sure that's much of an improvement over the PR though...

  2. Ideally we could just pass in context.res. There's some evidence this works here and it was included in the readme previously. But if it works then I'm not sure why @leecade would decide to do it the current way (mock response object) in the first place. In other words it probably needs plenty of tests.

  3. Don't delegate at all, just fork and rewrite the middleware for Koa. Obviously not desirable for various reasons.

  4. Get the original middleware to include a Koa-compatible version, or maybe just add some helpful hooks. Problem with this is that the maintainer isn't very amenable to the idea: added support for koa middleware webpack-contrib/webpack-hot-middleware#123

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

Successfully merging a pull request may close this issue.

2 participants