Settings set in `app.defaultConfiguration` is not inherited #1637

Closed
tellnes opened this Issue May 23, 2013 · 23 comments

Comments

Projects
None yet
8 participants

tellnes commented May 23, 2013

Is this intended behavior?

var express = require('express')
  , assert = require('assert')

var parent = express()
var child = express()

parent.enable('something')
parent.disable('x-powered-by')

parent.use(child)

assert(parent.enabled('something') == true)
assert(parent.enabled('x-powered-by') == false)

assert(parent.enabled('something') == child.enabled('something'))
assert(parent.enabled('x-powered-by') == child.enabled('x-powered-by')) // fails
Owner

tj commented Jun 2, 2013

hmm we may be inheriting at the wrong time, that would explain the express-defined ones being reset. It's a bit of a leaky concept in the first place unfortunately since one app could be mounted in many places / to many apps :s but I'd definitely consider this a bug

Hey!! that you turned off the 'x-powered-by' before said 'Express'?

timmywil commented Sep 3, 2013

It would be great if this was fixed. Re-setting these in all sub-apps is tedious.

behrang commented Oct 21, 2013

+1

behrang commented Nov 12, 2013

It seems that some settings are seen by child app and some aren't. trust proxy is inherited, but x-powered-by is not!

elcct commented Nov 13, 2013

I don't think this is a bug, but rather a design issue. Any property that is set after object has been created will override the property of the object that you want to inherit from.

Example code:

function A() { this.settings = {}; }

var a = new A();
var b = new A();

a.settings.foo = true;
b.settings.__proto__ = a.settings;

console.log(b.settings.foo); //true

a.settings.foo = false;
console.log(b.settings.foo); //false

b.settings.foo = true;
console.log(a.settings.foo); //false
console.log(b.settings.foo); //true

In our case following properties cannot be inherited from parent:
'x-powered-by', 'etag', 'env', 'subdomain offset', 'view', 'views', 'jsonp callback name', 'json spaces'

And any property that exists in parent and was changed in child object - basically each object has its own copy of settings that are not in "sync"

I have created a fix which adds a new setting called "settings use parent".

If you do:

child.enable('settings use parent');

and then:

parent.use(child);

In 'mount' event all settings that exist in parent will be deleted from the child and that will make parent's settings "visible".

'set' method will then set parent's settings instead of child's, so these will remain in "sync".

That's one of many possible solutions. Probably you don't want all settings to be inherited etc.

It's not a parent-child relationship. The "children" are modularized apps encapsulating functionality for the main app. They do not extend the main app when created. In OO terms, they do not have an animal-dog relationship. When these apps are added to the main app (using .use()), it makes no sense to me that they would override the settings of the main app, which is what is happening. Practically speaking, wouldn't you like the same settings shared across all of these apps? I don't foresee any use cases where one would want to disable the "x-powered-by" header only for a certain number of apps. If disabled, it is much more helpful to keep it disabled everywhere.

Contributor

defunctzombie commented Nov 13, 2013

I actually don't see the bug here. Apps are self contained and it makes sense to me that setting something on a parent vs a child will produce different results in the apps. This is why I mount routes instead of apps.

@defunctzombie TJ at one time recommended that you not do that and mount apps instead. As I said, this is not really a parent-child relationship.

elcct commented Nov 13, 2013

Maybe it's better to call it master - slave relation then. In this case modification of 'set' method that will pass the call to the "master" could solve it - that way "slave" once mounted will use "master" settings as its own.

Owner

tj commented Nov 13, 2013

some things definitely should propagate, like "trust proxy" is a big one

Contributor

defunctzombie commented Nov 13, 2013

I think there should be a difference between an app (of which there is only one) and things you mount into it. If all your main app is doing is mounting a bunch of little apps, then you don't have a main app, you have a proxy. Likewise, if all your little apps are really just routes, then they should be routes and not apps.

elcct commented Nov 13, 2013

How about separate settings container that you could pass via app constructor then?

Member

jonathanong commented Nov 13, 2013

i think app.use() should expect an independent (req, res, next) signature function (which an app is). it should not inherit any settings.

then have a separate function just for mounting apps with settings inheritance, i.e. app.mount(app).

right now, we're mixing the two philosophies. app.use() should be as simple as possible.

elcct commented Nov 13, 2013

I second that

@jonathanong Good suggestion 👍

tellnes commented Dec 3, 2013

I've written a module which I use to work around this.

https://github.com/tellnes/express-inherits

Member

jonathanong commented Dec 3, 2013

@defunctzombie perhaps we should make an easier way to make a new router and direct the documentation to users using router instances instead of new apps. i know this is your thing. then we won't have to deal with this problem. something like var subapp = express.router(), though we shouldn't call it any sort of app.

Contributor

defunctzombie commented Dec 3, 2013

That's what I already do in my apps :)

The only thing routers don't have is a .use and for that I've just been
doing a get('*') where I need it.

I actually put routers in files and just export the middleware function. I
dont call it a subapp, just typically:

app.use('/foo', require('./routes/foo'));

Or something like that.
On Dec 3, 2013 2:13 AM, "Jonathan Ong" notifications@github.com wrote:

@defunctzombie https://github.com/defunctzombie perhaps we should make
an easier way to make a new router and direct the documentation to users
using router instances instead of new apps. then we won't have to deal with
this problem. something like var subapp = express.router(), though
there's got to be a better name for that.


Reply to this email directly or view it on GitHubhttps://github.com/visionmedia/express/issues/1637#issuecomment-29688056
.

Member

jonathanong commented Dec 3, 2013

oh i know you do! i'm saying we should push people to do that instead of mounting apps and make it easier for them to do so.

Contributor

defunctzombie commented Dec 3, 2013

I guess it just requires some examples? Other than router.use being missing (if that is even an issue) I don’t see what else we have to do?

On December 3, 2013 at 3:40:07 PM, Jonathan Ong (notifications@github.com) wrote:

oh i know you do! i'm saying we should push people to do that instead of mounting apps and make it easier for them to do so.


Reply to this email directly or view it on GitHub.

Member

jonathanong commented Dec 3, 2013

yeah docs, examples. i think var router = express.router() would be nicer. we can alias router.use() as router.all() just so it works - i think you had an issue for that.

Member

jonathanong commented Dec 4, 2013

going to close this an open another issue.

FYI guys: if you want settings to be inherited, don't mount apps, mount routes. You can currently make a new router by doing new express.Router()

jonathanong closed this Dec 4, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment