extending options from a prototype

[jonathanong]

i currently have an issue when creating http.client() requests from an options object that is created from a constructor: petkaantonov/urlparser#5

https://github.com/iojs/io.js/blob/v0.12/lib/_http_client.js#L50

my solution would be options = Object.create(options || {}) or something, but i'm pretty sure someone's going to complain about performance or something...

what do you guys think?

[rvagg]

perhaps we can have a test case for this to find a more optimal path, perhaps one that avoids Object.keys() if that's the root problem

[chrisdickinson]

This is a somewhat contentious issue.

IMO, I agree with the current behavior -- Node/IO apis should only take the given object into account, and ignore its [[Proto]] chain. That said, the TC should probably make an authoritative decision about "how Node/IO deals with API parameters" -- ideally something that can be put into the docs.

[rlidwka]

I'd rather support proto chain, because sometimes it's very convenient to have. Imho, this proto getters case is a very strong argument to do so.

According to my benchmarks, Object.create() is 2 times faster than util._extend() on options object with no properties and the more properties there are, the worse util._extend() gets.

url.parse() output object contains 12 properties, and Object.create() outperforms util._extend on it 6 times (4.2M ops/sec vs 0.6M ops/sec).

So it's faster even if we're talking plain js objects. But urlparser tries to improve speed with lazy getters, and current util._extend() implementation practically forbids you to do this kind of performance optimizations.

This is the reason I'm usually using Object.create() whenever I'm trying to modify options object leaving original function arguments intact.

I suppose this is an old code, and v8 didn't have Object.create() at the time when it was written. That's why we never give much thought to this. But I think it's about time to discuss what the best practice here is.

[chrisdickinson]

I suppose this is an old code, and v8 didn't have Object.create() at the time when it was written. That's why we never give much thought to this. But I think it's about time to discuss what the best practice here is.

I do not think it would be safe to assume that the absence of its usage in Node's APIs for options objects is due to the lack of Object.create. Object.create (and polyfills for Object.create) have been around for a long time.

url.parse() output object contains 12 properties, and Object.create() outperforms util._extend on it 6 times (4.2M ops/sec vs 0.6M ops/sec).

While performance has always been a guiding concern of Node, there has also long been a desire to make the API more explicit where possible, even at the cost of CPU cycles. I can envision a situation where {host: 'some-host'} is linked to an object specifying hostname, and requests end up going to an unexpected location. Trying to debug that would be more than a little maddening, since even console.log'ing the object is not going to display the parent object's hostname property.

[rlidwka]

I can envision a situation where {host: 'some-host'} is linked to an object specifying hostname

I did shoot myself in a foot once, when I had an option object like opts={uri: 'foo'} and was setting opts.url='bar' after that (request library supports both). No prototypes involved.

The fact that you can specify the same thing in two different ways is an api issue, which has nothing to do with prototype chains imho.

Trying to debug that would be more than a little maddening, since even console.log'ing the object is not going to display

I suppose an addition to console.log which says "hey, this object has non-standard prototype (for example, instead of { host: 'blah' } displaying {* host: 'blah' } or < host: 'blah' > or { host: 'blah', <proto-chain-length>: 2 } would help this a bit.

[davidmarkclements]

If you outlaw the proto chain on options objects, it will cause breakage on legacy code.

I wonder if in the specified case, Object.create(Object.freeze(options || {})) might cause V8 to optimize the case (eg it can rely on static rather than dynamic lookups on the proto chain) - and if not it may be a case that v8 optimize in the future

[domenic]

I feel somewhat strongly that you should always follow the prototype chain, i.e. just use [[Get]]. This is the most natural pattern, i.e. var x = options.x.

Another way of thinking about this is that if you were to write such options-taking functions in ES6 you would likely do

function client({ agent, protocol, ... }) {
  /* use agent, protocol, etc. */
}

Probably http.client itself couldn't work this way because it has so many overloads and defaults, but it should still be useful semantic guidance.

[rvagg]

From TC meeting, #144:

@bnoordhuis: continue in the issue tracker

@indutny and @piscisaureus also expressed opinions on this but there was no resolution, just an agreement that there shal be further bikeshedding. I'm going to remove the tc-agenda label for now but if this keeps on going on and doesn't get anywhere productive then the label could be put back for a future meeting.

[jonathanong]

feels good to know you guys discussed this at least. 🙌 io.js 🙌

[dougwilson]

/cc @moll

[chrisdickinson]

Closing this. There's no definitive resolution from the TC, but @domenic swayed me to the proto-side via the ES6 destructuring argument. If anyone notices any specific APIs that are not accepting inherited attributes, feel free to open a PR and cc me. I'd be happy to take a look at them, and am generally +1 on making those sorts of changes.