tandrewnichols commented 9 years ago

We recently upgraded lineman from 0.29.0 to 0.34.1. Previously, using req.get('host') in our app returned the correct host (for us "local.manta.com:8000"). With the upgrade, it now returns "localhost:3030". Is this a bug that was introduced between these two versions or a config change that we just missed?

searls commented 9 years ago

Maybe the changeOrigin PR? See recent releases and try flipping that option

Otherwise could you please bisect this issue by installing more intermediate lineman versions to find when this was introduced?

On Mon, Sep 22, 2014 at 1:34 PM, Andrew Nichols notifications@github.com wrote:

We recently upgraded lineman from 0.29.1 to 0.34.1. Previously, using req.get('host') in our app returned the correct host (for us "local.manta.com:8000"). With the upgrade, it now returns "localhost:3030". Is this a bug that was introduced between these two versions or a config change that we just missed?

Reply to this email directly or view it on GitHub: https://github.com/linemanjs/lineman/issues/323

tandrewnichols commented 9 years ago

Setting changeOrigin: false fixes the problem. Is that the desired solution or should it just work without that option (as it has previously)?

searls commented 9 years ago

That changeOrigin PR claimed it was previously impossible to set it to false. Yet what you're saying is that false was the old default? I'm confused. Could you @mention the original author in this thread?

On Mon, Sep 22, 2014 at 1:52 PM, Andrew Nichols notifications@github.com wrote:

Setting changeOrigin: false fixes the problem. Is that the desired solution or should it just work without that option (as it has previously)?

Reply to this email directly or view it on GitHub: https://github.com/linemanjs/lineman/issues/323#issuecomment-56411965

tandrewnichols commented 9 years ago

We previously did not have changeOrigin set at all. Our server config looked like:

    server: {
      pushState: false,
      apiProxy: {
        enabled: !isTest,
        port: 3030
      }
    },

and we had no problem with host. With lineman@0.34.1, this config does not work, but adding changeOrigin: false does. @chringwer

searls commented 9 years ago

thanks Andrew. I haven't read the code lately but I'm a bit annoyed as this is now a regression when I didn't think it had been.

On Mon, Sep 22, 2014 at 2:06 PM, Andrew Nichols notifications@github.com wrote:

We previously did not have changeOrigin set at all. Our server config looked like:
    server: {
      pushState: false,
      apiProxy: {
        enabled: !isTest,
        port: 3030
      }
    },
and we had no problem with host. With lineman@0.34.1, this config does not work, but adding changeOrigin: false does. @chringwer

Reply to this email directly or view it on GitHub: https://github.com/linemanjs/lineman/issues/323#issuecomment-56414269

chringwer commented 9 years ago

Well, this is exactly the issue I ran into when upgrading to 0.34.0.

The breaking change was introduced with #255. With my PR one can at least set changeOrigin back to false explicitly.

tandrewnichols commented 9 years ago

Interesting. So it sounds like the default going forward is going to have to be changeOrigin: false. Which, let's face it, is not a major inconvenience. But it does break backwards compatibility, and it does require new documentation. @searls, is this your read on things too?

searls commented 9 years ago

Christoph, I thought your PR was designed not to change the defaults though. Simply to make setting the opposite possible.

On Mon, Sep 22, 2014 at 2:43 PM, Andrew Nichols notifications@github.com wrote:

Interesting. So it sounds like the default going forward is going to have to be changeOrigin: false. Which, let's face it, is not a major inconvenience. But it does break backwards compatibility, and it does require new documentation. @searls, is this your read on things too?

Reply to this email directly or view it on GitHub: https://github.com/linemanjs/lineman/issues/323#issuecomment-56419995

chringwer commented 9 years ago

Correct. I have not touched the defaults. Just enabled users to revert to the default it was before you've merged that PR by @davemo (#255)

jasonkarns commented 9 years ago

In versions prior to 0.30.0, apiProxyChangeOrigin defaulted to undefined.

apiProxyChangeOrigin = grunt.config.get("server.apiProxy.changeOrigin") || undefined

255 attempted to change the default to `true` but instead made it impossible to set it to `false`

apiProxyChangeOrigin = grunt.config.get("server.apiProxy.changeOrigin") || true

320 fixed the bug introduced by #255, thus allowing users to set it to `false`. However, the intent of #255 was to change the default to `true`, which #320 maintained.

apiProxyChangeOrigin = grunt.config.get("server.apiProxy.changeOrigin")

Summary, #255 introduced a backwards-incompatible change by changing apiProxyChangeOrigin's default from undefined to true.

tandrewnichols commented 9 years ago

Thanks for that summary. Maybe it's my personal bias because of our needs, but it feels to me like the default should be false. When it's false, it behaves more like a non-lineman environment would. That is, we use req.host and it behaves AS EXPECTED in all environments. Changing that feels like non-default configuration that should be specified, especially as it's non-backward compatible. But this isn't my repo and therefore not my decision.

searls commented 9 years ago

Jason, "backwards-incompatible" is maybe a bit strong? Shouldn't #255 just set it to false? I think we misread the OP's problem when he said he couldn't set the value to false to mean that the default was true (and it wasn't)

On Tue, Sep 23, 2014 at 9:40 AM, Andrew Nichols notifications@github.com wrote:

Thanks for that summary. Maybe it's my personal bias because of our needs, but it feels to me like the default should be false. When it's false, it behaves more like a non-lineman environment would. That is, we use req.host and it behaves AS EXPECTED in all environments. Changing that feels like non-default configuration that should be specified, especially as it's non-backward compatible. But this isn't my repo and therefore not my decision.

Reply to this email directly or view it on GitHub: https://github.com/linemanjs/lineman/issues/323#issuecomment-56520692

jasonkarns commented 9 years ago

@searls the intent of #255 was to change the default to true, when the default was previously undefined. I'd say that's backwards-incompatible because any lineman upgrade would automatically see different behavior without changing their config.

tandrewnichols commented 9 years ago

Case in point, it changed for us.

searls commented 9 years ago

Andrew, I'm going to go ahead and close this now that I better understand that #255 was an intentional breaking change. All to say sorry that the new default wasn't what you guys needed and that we failed to communicate this clearly in upgrade notes :(

tandrewnichols commented 9 years ago

That's fine. No problem. Thanks.

linemanjs / lineman

Did something change with host headers from apiProxy? #323

Setting `changeOrigin: false` fixes the problem. Is that the desired solution or should it just work without that option (as it has previously)?

and we had no problem with host. With lineman@0.34.1, this config does not work, but adding `changeOrigin: false` does. @chringwer

Interesting. So it sounds like the default going forward is going to have to be `changeOrigin: false`. Which, let's face it, is not a major inconvenience. But it does break backwards compatibility, and it does require new documentation. @searls, is this your read on things too?

255 attempted to change the default to `true` but instead made it impossible to set it to `false`

320 fixed the bug introduced by #255, thus allowing users to set it to `false`. However, the intent of #255 was to change the default to `true`, which #320 maintained.

linemanjs / lineman

Did something change with host headers from apiProxy? #323

Setting changeOrigin: false fixes the problem. Is that the desired solution or should it just work without that option (as it has previously)?

and we had no problem with host. With lineman@0.34.1, this config does not work, but adding changeOrigin: false does. @chringwer

Interesting. So it sounds like the default going forward is going to have to be changeOrigin: false. Which, let's face it, is not a major inconvenience. But it does break backwards compatibility, and it does require new documentation. @searls, is this your read on things too?

255 attempted to change the default to true but instead made it impossible to set it to false

320 fixed the bug introduced by #255, thus allowing users to set it to false. However, the intent of #255 was to change the default to true, which #320 maintained.

Setting `changeOrigin: false` fixes the problem. Is that the desired solution or should it just work without that option (as it has previously)?

and we had no problem with host. With lineman@0.34.1, this config does not work, but adding `changeOrigin: false` does. @chringwer

Interesting. So it sounds like the default going forward is going to have to be `changeOrigin: false`. Which, let's face it, is not a major inconvenience. But it does break backwards compatibility, and it does require new documentation. @searls, is this your read on things too?

255 attempted to change the default to `true` but instead made it impossible to set it to `false`

320 fixed the bug introduced by #255, thus allowing users to set it to `false`. However, the intent of #255 was to change the default to `true`, which #320 maintained.