special handling of app.set() config objects #1113

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants

Previous behavior was to overwrite objects, but it makes much more sense
for a configuration file to merge the new object with the existing one.

This allows for things like:

 app.set('tv settings', { channel: 42 });
 app.set('tv settings', { volume: 100 });
 app.get('tv settings');
 // => { channel: 42, volume: 100 }

This seems much more intuitive, and avoids confusing bugs like:

app.configure(function(){
    app.set('view options', { layout: false });
});
app.configure('development', function(){
    app.set('view options', { pretty: true });
});
// => In development config, layout: true !?!?

But it's a public API change. Conceivably some websites may rely on the above behavior. But it seems really insidious and misleading, and probably the following is more common:

app.configure(function(){
    app.set('view options', { layout: false });
});
app.configure('development', function(){
    app.set('view options', { layout: false, pretty: true });
});

which won't break from these changes -- it will just be redundant.

special handling of app.set() config objects
Previous behavior was to overwrite objects, but it makes much more sense
for a configuration file to merge the new object with the existing one.
This allows for things like:

     app.set('tv settings', { channel: 42 });
     app.set('tv settings', { volume: 100 });
     app.get('tv settings');
     // => { channel: 42, volume: 100 }
Owner

tj commented Apr 26, 2012

I dont entirely disagree, but this introduces more complexity than just knowing that it simply sets the value to whatever you pass. For example it would then try and go merge two database objects if you had app.set('db', redis.createClient()) or similar. Even if we add some hacks to get around that it feels a bit weird to me. In the future set/get will probably be for internal settings only (express 4.x). This would influence people to choose what suits them best, be it json config, hybrid env/redis/json config etc. I'll think on this though

To be affected by the merging behavior, both the new and existing values must be objects. For example:

app.set('db', redis.createClient());
// .... later ....
app.set('db', mongo.createClient());

I would guess this is an uncommon case. Yet, it's true that merging may not be the intended behavior.

If this patch isn't appropriate, here are a few other options:

  1. create a new method for this: app.set_merge('tv', { channel: 42 });
  2. a leading '+' in the setting means merge: app.set('+tv', { channel: 42 });
  3. create a new method for the old behavior: app.reset('db', redis.createClient());
Contributor

benjamin-atkin commented May 7, 2012

I have an idea: add a new class for computed settings (a la Knockout.js), and do an instanceof check on a two-argument call to set(). If it's the computed settings class, have that take over the getting and setting from then on. Then it would go like this:

// express.computedSetting returns a new subclass of Express.ComputedSetting
var MergeSetting = express.computedSetting({
  write: function(current, new) {
    for (var key in new) {
      current[key] = new[key];
    }
  }
});

app.set('tv settings', MergeSetting); // this adds MergeSetting to a hash of setting handlers
app.set('tv settings', { channel: 42 }); // this invokes MergeSetting
app.set('tv settings', { volume: 100 });
app.get('tv settings');
// => { channel: 42, volume: 100 }

This could also be a plugin, and would just require a simple hook, I think, where it could return true if it decides to handle a call to set() and false if it wants express to handle it. Or maybe just overriding set() would work.

Member

jonathanong commented Sep 10, 2013

closing because it sounds like tj doesn't want it

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