Added app.extend()

[boycce]

Added app.extend(), which provides an easy way to update certain properties of a setting.
Example:

app.set('foo', { 'bar' : 1 })
app.extend('foo', { 'baz' : 1 });
app.get('foo');
// => { "bar" : 1, "baz" : 1 }

Related issue #3499

[wesleytodd]

Hey @boycce. This is an interesting addition. Can you provide some real world examples of why you would like this?

Could you also take a look at how you would do this addition considering this planned change: #3218

[boycce]

Hey @wesleytodd I see no issues migrating .extend() along with the other getter/setter methods.

One example would be if you needed to update stored configurations for a particular service while keeping to express's familiar set/get/expand* methods:

app.set(braintree, {
  'environment': 'production',
  'merchantId': 'os9ksSioSeyH87fhd',
  'publicKey': 'kcYsjcSlkfd8Hs2jd',
  'privateKey': '1z844211b3d012228dee47279ef30000',
  'merchantAccountIdNZD': 'userNZD',
  'merchantAccountIdUSD': 'userUSD'
})

app.expand('braintree', {
  'privateKey': '3syd7D9l02hh11dB91p02kdufhnw712a'
})

Maybe @brunoscopelliti could also share his use case for needing extend().

[brunoscopelliti]

In some cases I found useful to set related fields under the same object.
As example:

app.set("log_level", "verbose");
app.set("log_active", true);

// vs 

app.set("log_config", { level: "verbose", active: true });

When I use the latter approach I would like to have a better method to update a single field, than:

app.set("log_config", Object.assign({}, app.get("log_config"), { dir: "/db/log/" }));

[wesleytodd]

Hey @boycce, first off, I hope those are not your production configs ;). But second @boycce & @brunoscopelliti, your use cases seem reasonable. I prefer extend over expand, assuming that is not just a typo.

I think this change is something which could be landed on 4.x as well as the 5.x branch. So can you update this PR to point to master for 4.x, then create a PR to store-settings so that I can update #3218 with this?

[dougwilson]

Is this handling inherited settings correctly? It seems like it's actually altering the parent property in addition to setting the child property to the same object instance. I feel like the parent object should not be getting modified, so the setting on the parent app should remain the same when the setting on the child app is extended.

[wesleytodd]

I dont think it would set the parent's property because it is prototypicaly inherited, not done by reference. So if it was initially from the parent, it would just result in settings it's own property with a fully copy of the parent and not touching the parent values. I could be wrong though, would be worth having a test around this.

See: https://github.com/expressjs/express/pull/3568/files#diff-5372f626ee15242f1e2c6eb31655b4faR116

[dougwilson]

I did test this and it is altering the parent property as well as setting the child property to the same object reference.

[wesleytodd]

Ahh, ok then I miss-read how this works, sorry. Yes I agree it should not modify the parent.

[dougwilson]

Here is the test I ran. I feel like this test should pass, but it currently does not:

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

var app1 = express()
var app2 = express()

app1.set('log_config', { level: "verbose", active: false })

assert.equal(app1.get('log_config').active, false)
assert.equal(app2.get('log_config'), undefined)

app1.use(app2)

assert.equal(app1.get('log_config').active, false)
assert.equal(app2.get('log_config').active, false)

app2.extend('log_config', { active: true })

assert.deepEqual(app1.get('log_config'), { level: "verbose", active: false })
assert.deepEqual(app2.get('log_config'), { level: "verbose", active: true })

console.log('success')

assert.js:81
  throw new assert.AssertionError({
  ^
AssertionError: { level: 'verbose', active: true } deepEqual { level: 'verbose', active: false }
    at Object.<anonymous> (test.js:19:8)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.runMain (module.js:604:10)
    at run (bootstrap_node.js:389:7)
    at startup (bootstrap_node.js:149:9)
    at bootstrap_node.js:504:3

[boycce]

Updated to 4.x and removed object referencing.
Would you like me to support Node 3.3 and below concerning Object.assign()

[wesleytodd]

I left a comment about the isArray usage, but the same does apply to Object.assign.

[dougwilson]

Instead of adding more code here, can the dependency utils-merge we already have perform this function? Or if not, what's the difference between this code and utils-merge?

[boycce]

Hi @dougwilson, sorry I didn't see this merge util, I was scanning for an extending extend() function or corresponding library 🤐. This passes my tests:

this.settings[setting] = merge(merge({}, this.settings[setting]), val)

[wesleytodd]

Ugh, I didn't realize that utils-merge did not support variadic calls. I think this is, sadly, the best way at the moment, but I think we should add to the roadmap "removing utils-merge in favor of object.assign or an equivilant fallback".

[dougwilson]

What do you think we should do for the case where .extend is called on a setting that is already set but is not an object? Right now it is silently overwritten. Do we think that is the correct thing to do?

Example:

$ node -pe 'require("express")().set("thing","foo").extend("thing",{"foo":"bar"}).get("thing")'
{ foo: 'bar' }

[dougwilson]

@wesleytodd thoughts on this? I'm not sure what the use of having .extend called with one argument returning the current value -- that's already covered with .get and .set I don't think we need a third method to get the value of a setting, right?

[wesleytodd]

Agreed. Not sure if it should error, but it shouldn't do anything and shouldn't be documented.

[dougwilson]

May be useful to throw error that required argument is missing.

[boycce]

OK, I will edit this later today.

[dougwilson]

This should probably be a TypeError

[dougwilson]

I would recommend Array.isArray instead of instanceof Array. All the instanceof Arrays were removed in Express 1.0.

[wesleytodd]

When looking at node.green, I think in my other comment I mentioned that I thought that wasn't supported in node 0.12, but I think I looked at the wrong field, and it has been so long since I have used 0.12 I couldn't remember what was supported...

http://node.green/#ES2015-subclassing-Array-is-subclassable-Array-isArray-support

[wesleytodd]

What do you think we should do for the case where .extend is called on a setting that is already set but is not an object

I think it should refuse to work on any property which is not a plain object.

[dougwilson]

I think it should refuse to work on any property which is not a plain object.

Ah, that is a good point. If a setting is an instance, calling extend here would remove the original prototype, so yea, we don't want to destroy the object. We can always add additional things to extend as semver minors so just only working on plain objects sounds like a good first implementation.

[boycce]

@dougwilson, would you like me to add a isPlainObject export to the utils file, or require it from the lodash module in regards to checking if the object is in fact a plain object from the Object constructor.

I could use something like this:

function isPlainObject(obj) {
  return  typeof obj === 'object'
    && obj !== null
    && obj.constructor === Object
    && Object.prototype.toString.call(obj) === '[object Object]'
}

[wesleytodd]

My point was more along the lines of, "don't try to merge arrays" and "don't try to handle complex types". In re-reading it I see where this led, and I think I was making it too complicated.

So adding a dep for this is out of the question IMO. And there is a bunch of valid use cases for merging "object like" things, so the full isPlainObject is also not a good path forward. I would think just doing this is the best:

app.extend = function(setting, val) {
  if (val === null || typeof val !== 'object' || Array.isArray(val)) {
    throw new Error('value needs to be an object');
  }

  debug('extend "%s" with %o', setting, val);

  var currentSetting = this.settings[setting];

  if (typeof currentSetting === 'undefined') {
    this.settings[setting] = val;
  } else {
    this.settings[setting] = extendObject({}, currentSetting, val);
  }

  return this;
};

Thoughts?

[boycce]

Sounds good by me.

[dougwilson]

So, this pull request is a bit stale, and looking back, it doesn't seem like there was really a consensus about adding this to core express. As pointed out in the conversation above, there are certainly different extending strategies one can make... And with things like the prototype pollution issues going around and the fact that, at it's core, it's not really much more than app.set('foo', Object.assign(app.get('foo') || {}, { foo: 'bar' })) but without the flexibility to perform your own merge strategies, it just seems more complexity into core than it's worth...