Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

Add RFC forwarded header as hint to backend server #1139

Merged
merged 1 commit into from
Sep 26, 2018

Conversation

timkelty
Copy link
Contributor

@timkelty timkelty commented Sep 25, 2018

Fixes #1089

changeOrigin: true

Quickly looking around, it seems like it causes confusion no matter which way you default it:

Originally I was inclined to remove it just to keep the defaults from webpack-dev-sever, as I was the one who added it, and since am not sure why I needed it 😝

However, it seems like perhaps it is more useful as true for more people?

CRA, for example, sets this to true, but then again there are people asking that they don't.

So, I left it as-is. Seems maybe like a toss-up. Any input welcome.

X-Dev-Server-Proxy/ Forwarded

This PR removes the invented/non-standard X-Dev-Server-Proxy in favor of a Forwarded header to indicate to the proxied server that this request has been proxied by webpack-dev-server.

I think I've read this too many times and proxies, being proxies, confuse me quickly…
I'm not 💯 if this should be for: or by:. Anyone good at reading spec-language? 😁

@timkelty
Copy link
Contributor Author

@neutrinojs/core-contributors can anyone do a sanity-check and verify we should be using by: and not for: here?

@edmorley
Copy link
Member

So, I left it as-is. Seems maybe like a toss-up. Any input welcome.

CRA, for example, sets this to true, but then again there are people asking that they don't.

So, I left it as-is. Seems maybe like a toss-up. Any input welcome.

I think we should leave it enabled. There seems to only be one person asking for CRA to change the default, and presumably given the issues mentioned in some of the other linked issues/SO posts, there would be many more filed for the reverse, were CRA to ever change their default.

can anyone do a sanity-check and verify we should be using by: and not for: here?

Reading the spec I'm pretty sure by: is the correct one :-)

Copy link
Member

@edmorley edmorley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@edmorley edmorley merged commit 9f60e75 into neutrinojs:master Sep 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

3 participants