Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Node stdlib should take inherited properties into account and do so consistently #7587

Closed
moll opened this issue May 8, 2014 · 25 comments
Closed

Comments

@moll
Copy link

moll commented May 8, 2014

Hey,

Inherited properties are a core part of JavaScript and can be used to great extent beyond just for classes. Unfortunately there are inconsistencies with regards to how inherited properties are handled when passed to various functions or classes in the Node stdlib. It's a case where the the implementation leaks to the public API and the main culprit is Util._extend.

Util._extend is implemented to ignore inherited properties of the source (2nd param) and that creates hidden and undocumented public API limitations and difference. Functions using Util._extend will ignore inherited properties while those doing things in the traditional way with for in do not. The common practice of checking for truthyness with if (options.handle) ... of course does take inherited properties into account, so the status quo is bound to remain inconsistent.

I propose the API is made consistent by taking inherited properties into account everywhere.
This may entail changing Util._extend, too, to iterate over all properties, but fortunately it's a private function and those people who expect various Node stdlib functions to ignore inherited properties are treading on thin ice anyways.

If the proposition is okay, I can submit a patch/PR with tests.

@moll
Copy link
Author

moll commented May 8, 2014

Why should an implementation detail like Object.assign affect the public API of functions? Don't look at it only from the point of a private function called Util._extend. Look at it from the whole point of APIs. Why should inheritance from objects be actively removed? What is the problem to which removing inheritance is the solution? Whose problem is it? Is it the callers problem or the callees problem?

Object.assign influences Lodash's _.assign as you provide a shim. Great. That has nothing to do with how the language in general works (e.g. property access) or how modules should present their APIs.

Most punt on Object.prototype extensions it's not a real problem.
Mind explaining how Object.prototype relates to this? I agree, Object.prototype must not be extended. Fortunately one can inherit directly from objects in JavaScript — one of the greatest feature in prototypical languages.

I think we must define common, though. I took a random example — Socket in net.js. It does no such extend thing and checks options.handle. You're saying it should specifically remove the [[Prototype]] from options?

@moll
Copy link
Author

moll commented May 8, 2014

If devs want to work with objects and inheritance Object.create is the way to go. Expecting every API point to cater to inheritance and the baggage it brings is not practical.

What is the baggage that passing in an inherited object brings? The moment you want to make a copy of it in your own function for mutation, you'll iterate over the properties and flatten it out.

I don't know which school of design you adhere to, but inheriting from T should be an implementation detail in functions which say they take a T. Otherwise they must make it explicit that T must not be inherited from. Inheritance is so in-grained in the visible part of JavaScript via property access etc. that it's extra work to fight that. And why would you, I don't understand.

A quick check — are we arguing over the same thing? I say the following code must work consistently:

var DEFAULT_OPTIONS = {host: "example.com"}
var opts = Object.create(DEFAULT_OPTIONS)
opts.port = 443
new Socket(opts)

Are you're saying that the caller would have to flatten opts out every time? Or only when they know that the implementation of a particular function uses an extend function which ignores inheritance?

@moll
Copy link
Author

moll commented May 8, 2014

I have a feeling we or I am not putting enough emphasis on prototypical inheritance in this discussion. JavaScript is not a _class_ical language, so all there is is inheritance from existing objects. The fact that inheritance is under-used in non-constructor cases, I can imagine, is both due to Object.create being somewhat new and due to developer ignorance. I can't change the past, but I'm definitely going to do as much as I can to educate people to think outside of the classical world view. People are already coming to grasp closures. I'm sure they can grasp prototypes, too, especially how clear Object.create makes them compared to new MakeNewObjectFromPrototype.

When working with inheritance you should subclass.

Subclass what? I assume you meant inherit, given that JavaScript has no classes? So in other words when working with inheritance, one should inherit? 8-)

I don't find flattening out inheritance to be intuitive.
Flattening inherited properties creates bizarro cases, for example when shallow cloning properties I wouldn't expect inherited properties to be flattened.

That could only be if you think your function has any job to care how the object given to you is implemented. It doesn't. It shouldn't. I give you an object with options set in properties. That's should be our contract.

While our previous hot discussion was around Lodash's utilities, let's leave implementing and naming cloning functions to another day. This is about public APIs and their contracts.

It looks like you're wanting extra work to support it.

Are you saying you'd rather have things be inconsistent and change sporadically dependent on some implementation detail of how default options are handled in that function? And/or specifically state that prototypes in a prototypical language are so complicated that we're going to fight with them by ensuring each plain object is run through a filter to remove its parents?

The status quo is a sunken cost and, as I said, I am willing to do the work myself. I've got a vested interest in this and care for consistency and non-surprising behavior.

I'm saying mucking with inheritance for things like options objects isn't needed.

As I asked above, people should not use prototypes in a prototypical language? ^_-


Mind clarifying for me what is your argument for the desire to disallow objects with inherited properties in (Node.js' or other's) APIs? So far I've only heard that it's because there's this Object.assign function which will arrive sometime in the future. How can one standard library function dictate public APIs of other code escapes me.

@moll
Copy link
Author

moll commented May 9, 2014

I'm riding the line between ES5/6.

As I've understood the "class" syntax for ES6 is only syntatic sugar, so all discussions around prototypical inheritance still apply. But in the end I'm not talking about classes, I'm talking about objects with inherited properties.

As I asked above, people should not use prototypes in a prototypical language? ^_-

Right. In this case it's not necessary and can cause extra wtf.

That's a little circular here. It causes extra wtf only because developers have used wtf-inducing helper functions — the idiosyncratic extends or possibly Object.keys. Other stick to the basics with for in or don't strip inheritance before using regular property access (obj.foo) and everything's a-okay.

Saying it's not necessary is a little meaningless. It solves a great problem of defaults without having to use external libraries, is the core part of prototypical inheritance and a valid technique. We could say the _ libraries are not necessary either, but they're just as valid tools as prototypes are. After all, the use case of defaults is in the name prototype: the first, original, or typical form of something.

Devs were doing something like Object.defineProperty(o,'foo',{'value':'bar'}) because missing descriptor attributes are considered false. [...]

So there were some dudes who extended Object.prototype or set descriptors wrong on their own object and now because of that we're writing inheritance off wholesale? What? That's their problem. And that was precisely why I asked "What is the problem to which removing inheritance is the solution? Whose problem is it?".

If your function wants to mutate the passed-in object, absolutely, make your own copy:

function api(options) {
  options = extend({}, options)
}

function extend(target, source) {
  for (var key in source) target[key] = source[key]
  return target
}

Making your own copy has nothing to do with how the caller set up their descriptors beyond that they made them enumerable.

I think it's better for your Options instance to have something like a toJSON, toObject, or value method that flattens it down to an object of own properties rather than change the existing behavior of util._extend.

First, that would be the caller who would have to remember to call those functions. And secondly, you're thinking in classical terms again. This is not about classes. This is about an object with inherited properties. There's no behavior attached to this object.

To remind, I'm not talking only about Util_extend, I'm talking about the whole contract Node's public APIs present. They're currently inconsistent with handling options values. I'm arguing to have them consistently accept plain objects regardless of their inheritance tree (an implementation detail). I'm even willing to do the work.

Surely you don't like inconsistency any more than I do, having touted it as a feature of Lodash just recently? There's work involved in making it consistent regardless. I would even say more work to ensure inheritance is stripped than to allow it everywhere.

@jdalton, why are you so opposed to something that will make no difference to your code, but will make things more consistent and flexible for others? If you've so far used only objects with own properties, nothing will break or change for you.

@moll
Copy link
Author

moll commented May 9, 2014

As I've understood the "class" syntax for ES6 is only syntatic sugar

Let it go >__>

With all due respect, if your argument is based on a (falsifiable) statement, then that statement may get criticized. Don't take it personally.

No, it's wtf-inducing because they assumed inheritance wouldn't play a part in it. The method should have performed HasOwnProperty checks instead of HasProperty checks.

Are you kidding me? There's a person who has an object with let's say silent set to true in the prototype, passes that into a function that will check if silence is set to true and then is surprised that that function read silence to be true? If so, come on, they had it coming.

If you're talking about someone adding enumerable properties to Object.prototype, then we've already established we agree that it's bad practice and must not be done. But making a copy of a given object with extend won't save from inheriting from Object.prototype anyways, so what's the fuss about?

No it's not. I mean the problem has been tackled w/o the need for inheritance.

That's funny, because one can say the opposite is in fact true — Object.create and/or a for in loop had solved the problems of extending even before Underscore arrived. So what, let there be more than solution. One is a subset of the other anyhow.

First, that would be the caller who would have to remember to call those functions

Naw, I'm saying before you pass them in you do options.toXyz().

Umm, that's a little contradictory. Where would that toXyz come from if not from shared behavior (functions in prototype) and who is it that calls toXyz if not the caller of the function receiving that object?

Then you're SOL. Inherited options properties are a hazard.

Because it's proven to trip up devs, waste their time jumping through hoops to avoid, and offers little benefit besides default options which can be handled in other ways.

Perfect! Mind explaining how accepting inherited properties trip up the devs using that API? The argument about someone using Object.defineProperty applies equally to non-inherited properties, so that mistake can't be induced by inheritance.

It's like saying because one bafoon cut his fingers off with a metal knife, everyone else should cut tomatoes with the butter knife they made in 2nd grade. If you get tripped up by inherited properties, fine, stick to whatever works for you. This is not about you. This is about doing the right thing in an API used by everyone by also following the robustness principle.

The thing is, it already works fine with inherited properties in some/most of the places. All I want is to ensure it will work consistently everywhere. Don't worry, I would add automated tests to make sure it stays that way.

@othiym23
Copy link

othiym23 commented May 9, 2014

As a bystander on this issue, I have been (and continue to be) struck by the lack of consideration and generosity shown by both parties in the discussion. There has to be an easier way to work this out.

@kessler
Copy link

kessler commented May 9, 2014

@othiym23 In my opinion, the generosity was extended by @jdalton and was depleted by @moll in a separate thread/issue which started this debate.

@seishun
Copy link

seishun commented May 9, 2014

I must say I'm on @moll's side here. So far the only argument against supporting inherited properties is that they're supposedly dangerous, but I haven't seen a valid rationale.

Default properties have been solved in other ways, ways that don't lend themselves to tripping up devs or creating more work for them.

I'd really like to see how.

@othiym23
Copy link

othiym23 commented May 9, 2014

Thanks for the context, @kessler (edited: sorry @glasser!). Not taking a side, just pointing out that the arguing felt especially combative & non-constructive.

@moll
Copy link
Author

moll commented May 9, 2014

As we've sidetracked into subjective interpretations, I absolutely welcome any comments on the specifics of my generosity through email. I think we both have done a fine job of avoiding ad hominem attacks with the occasional humor. Any statement or argument, mine or others', on the other hand, should never be considered sacred nor safe from criticism. Some people always take criticism of their ideas personally. Ces't la vie. So, sorry, @jdalton, you're not my target, only your beliefs. :-)

Your behavior has hurt your cause and galvanized me into not only reviewing my own libs, which thousands depend on, and ensuring inherited options properties are not supported but also bringing this to the standardization body and encouraging them to continue their efforts to remove support as well. When I set my mind to something I have a hell of a follow-through.

Does the standardization body have someone arguing for prototypes? If not, I think we'll meet again. ;-)

I'm rather certain it's impossible to banish prototypical inheritance anytime soon. The opposite is even worse, after all — meta-regress, well described in the 90's paper "SELF: The Power of Simplicity" by David Ungar and Randall B. Smith.

But come on, you may not like me and that's fine, but don't fight ideas based on who supports them. Ideas are independent from people.

I've given an examples of devs getting tripped up, so much so that the language itself is changing. I haven't seen a reason why allowing inherited properties is worth the baggage, that is worth the workaround cost devs have had to employ to get around inherited property issues in other APIs.

The example you gave is about trusting default property descriptors when using Object.defineProperty. What does this have to do with inherited properties? You can use prototypes without Object.defineProperty and you can use Object.defineProperty without prototypes. You're conflicting modifying Object.prototype with prototypes in general, for which none of the current extend functions are solution to anyhow. The solution to your linked problem is Object.create(null), which is orthogonal to using that object as a prototype.

You can do this in a couple ways. One way is to bolt an options to your exports so for example foo.templateSettings are the default options for foo.template. For constructors I've seen it done as Foo.options or Foo.defaults which then apply to the new Foo(options).

A side note, but never ever change anything on the exported object as a user of that library. At least not on Node.js. Node may reuse and resolve a require from a module deep down to your modified instance and now you've just changed that module's behavior without knowing it.

@moll
Copy link
Author

moll commented May 9, 2014

It's not about blanket being for or against prototypical inheritance. I love inheritance, it's fantastic. But in the case of options object it's caused nothing but dev pain.

I honestly do not understand. I may be thick, but I can not imagine any problems with getting a plain object that makes use of prototypes. I will read properties off of that object and I'll be happy. If I want to modify that options object, I'll go over every property there and copy it somewhere. Just like I do with an options object that does not make use of prototypes beyond Object.prototype.

If you don't want to explain it to me again, I understand. If you do, feel free to quote yourself. Or perhaps there's someone else in this thread who got what you're saying and can enlighten me.

A side note, but never ever change anything on the exported object as a user of that library.

Maybe, it works for Underscore and its 5,000+ dependents.

That's only by pure luck. If you and I create two modules both dependent on module X and we both modify module X's defaults with e.g. require("x").options = ..., we will (eventually) break each other when we're both required in the same Node.js process.

@moll
Copy link
Author

moll commented May 9, 2014

I replied to @jdalton via email which made him revise his comment above, to which I shall now just add that it might not be a well known fact, but Node.js/NPM does not ensure you and I get different instances of module X if our version requirements match. It's do with resolving cyclical dependencies, and perhaps just saving unnecessary computation, that a lot of modules might end up sharing an instance: http://nodejs.org/api/modules.html#modules_loading_from_node_modules_folders. NPM has a convenient command called dedupe to induce this.

If you want to discuss this further, I'd be happy to, but let's find another medium and not hijack this thread. Should we start our own debate club? ^_^

@moll
Copy link
Author

moll commented May 9, 2014

The discussion between you and I seems to have reached a standstill, but the issue itself is definitely open until Node.js core comes to a consensus and hopefully I can submit a few patches to make things consistent. ;-)

@seishun
Copy link

seishun commented May 10, 2014

I've given examples of devs getting tripped up, so much so that the language itself is changing.

Node's usage of _extend doesn't protect against unintended changes to Object.prototype (if that can even happen in a controlled environment), since the resulting object still inherits Object.prototype. And I haven't seen anyone complain about this.

You can do this in a couple ways. One way is to bolt on options to your exports. For example, foo.templateSettings are the default options for foo.template. For constructors I've seen it done as Foo.options or Foo.defaults which then apply to the new Foo(options).

The whole point of defaults is being able to deviate from them. I don't see how moving them to a separate file solves anything.

@seishun
Copy link

seishun commented May 10, 2014

Again, the Object.defineProperty case was a basic example of the issue to show how inherited options properties can have unexpected side effects.

No, Object.prototype is the only example of unexpected inheritance. Inheriting anything else requires explicit usage of new or Object.create.

Dealing with inherited options properties is a corner case that doesn't affect current util._extend because it uses Object.keys which is great.

Oh it does affect it. util._extend doesn't use Object.create(null) internally, thus it returns an object that inherits Object.prototype (even if you pass it an object that doesn't originally inherit anything!). Try this, for example:

env.js

console.log(process.env);

test-env.js

var fork = require('child_process').fork;
var options = Object.create(null); // doesn't inherit anything!!

fork('./env.js', [], options);

Object.prototype.env = {hello: 'world'};

fork('./env.js', [], options); // oops!

The example I gave in that snippet was to use a defaults options object property and bolt it on to a function or namespace.

Are we both talking about your own application-specific defaults, rather than the function's provided defaults? @moll has provided an example before, here it is slightly modified:

var DEFAULT_OPTIONS = {host: "example.com"}
var opts1 = Object.create(DEFAULT_OPTIONS)
var opts2 = Object.create(DEFAULT_OPTIONS)
opts1.port = 443
opts2.port = 80
new Socket(opts1)
new Socket(opts2)

@seishun
Copy link

seishun commented May 11, 2014

That depends on placement/location of whatever you're inheriting from.

How is placement relevant?

As you said Node.js punts on Object.prototype extensions. The util._extend method does not apply inherited properties of options objects because of its Object.keys(add) use.

I think you're missing my point, or I'm missing yours. You gave me an example of developers getting tripped up on Object.prototype, and I showed you how this already affects all of node's API, whether or not it uses util._extend. As I've already said, I've never heard about anyone getting tripped up on this in Node.js.

Yes, I'm aware of the inherited options properties example. My suggestions were alternatives to that.

Would you mind providing an alternative to the example I provided, since all your examples seem to have the defaults inside the function getting called.

@moll
Copy link
Author

moll commented May 11, 2014

The solution in search of a problem here is precisely the action against letting objects with inheritance through. If you, John, say most devs don't use them, then there's no harm letting them by; no need to take extra measurements (check hasOwnProperty etc.) to stop them.

Your first accusation in this thread was that I was dealing with hypotheticals. Now we're in a situation where neither Nikolai nor me get the problem or have seen an isolated example of the "trip up" you so warn us of and try to protect the world from.

Please give us an isolated code example of the problem of taking inheritance into account in such a way where the problem is only caused by inheritance from an intermediate object and can not happen if said cause is applied directly to the passed-in object. Please also be precise whose problem it is — the caller's or the callee's.

A.

On 11 May 2014, at 11:28, John-David Dalton notifications@github.com wrote:

No, Object.prototype is the only example of unexpected inheritance. Inheriting anything else requires explicit usage of new or Object.create.

That depends on placement/location of whatever your inheriting from.

Dealing with inherited options properties is a corner case that doesn't affect current util._extend because it uses Object.keys which is great.

Oh it does affect it. util._extend doesn't use Object.create(null) internally, thus it returns an object that inherits Object.prototype (even if you pass it an object that doesn't originally inherit anything!).

As you said Node.js punts on Object.prototype extensions. The util._extend method does not apply inherited properties of options objects because of its Object.keys(add) use.

Are we both talking about your own application-specific defaults,

I've shown different ways to handle default properties take your pick.

moll has provided an example before, here it is slightly modified:

Yes, I'm aware of the inherited options properties example. My suggestions were alternatives to that.

I do agree with @moll that consistency either way would be great. I always try to get something out of these long discussions; going tit for tat through multiple issue threads is exhausting. In this case I'm taking steps to make things more consistent in my own code and in the language spec, just on the side of not supporting inherited object properties. I like to follow language precedents and since my focus is on utility methods the one I referenced was Object.assign. This works great because the common case for options objects is passing object literals with properties on them. So I side with the emerging native API, cover the common case, & avoid edge case oddities.

Whether Node agrees or disagrees isn't something I'll lose sleep over. While I enjoy super edge topics as much as the next dev at the end of the day based on the mixed support in code I've seen I don't think most devs even consider inherited options object properties as a use case.

For me this issue feels like a solution in search of a problem.


Reply to this email directly or view it on GitHub.

@seishun
Copy link

seishun commented May 11, 2014

If inheritance is not-obvious/unexpected because it's buried in other modules.

I'm not sure what you mean by inheritance being buried somewhere. An object's prototype is set at the point of its creation. If the object is created in another module, then either that module was written by you, or you're using someone else's object. In the latter case, you're likely going to have other problems than inherited properties.

You're missing mine. The example I used was directly related to inherited options object properties, which I then rewrote w/o using Object.prototype after you got hung up on it. Yours was not.

I assume you mean the first snippet here. So you're saying someone might...

  • Write a class
  • Set a property writable on its prototype
  • Create an instance of that class

...and then be surprised that that instance has a property writable? It's not even the "gotcha-land" anymore, it's basic knowledge how the language works.

I already provided examples of default options objects bolted on to objects/functions (outside).

If you mean the second snippet in the same comment, then it's not what I meant. The function is external, you have no control over how it handles defaults.

@seishun
Copy link

seishun commented May 11, 2014

Inheritance is not always obvious. Take the DOM for example; it may not be obvious what HTMLSomeElement inherits from.

This is what I meant by using someone else's object. If you're passing anything other than an object literal as options, then you should know what you're doing and where that object is coming from.

I'm saying libs/frameworks add abstraction so inheritance may not be straight forward and may not be something you've written directly.

Again, why would you pass an object coming from libs/frameworks as options unless you know what you're doing?

I'm not sure what you mean. Having default options exposed gives devs direct control over how the function handles defaults. This is a bit of a side track though.

Yeah but we're talking about functions that don't expose their default options (i.e. all of node's API). I gave you an example of using inheritance to provide your own defaults.

no one has explained why this feature is needed

See above point.

@seishun
Copy link

seishun commented May 11, 2014

Abstraction makes things less obvious was my point.

And my point was that it's the dev's choice to abstract or not.

why it's worth the code/test/edge cost

I don't see much cost. If the function doesn't copy the object, there's nothing to do. If it does, then it uses _util.extend, which would be changed to copy inherited properties (although I'd prefer never copying objects at all). Do you have any specific edge cases in mind?

@moll
Copy link
Author

moll commented May 11, 2014

@jdalton:

Yes, I'm aware of the inherited options properties example. My suggestions were alternatives to that.

Your alternatives, fine or not, are not an argument against using inheritance for defaults. It's great you're versatile to come up with them, but the quantity of alternatives is not evidence of anything for any one.

@jdalton:

If inheritance is not-obvious/unexpected because it's buried in other modules or several deep in the inheritance chain.

My friend, inheritance is an implementation detail and the default values due to inheritance form the public contract of an export, so no surprises there.

@jdalton:

I'm not leading the charge here, the process to remove support for inherited options in spec had already started some time ago.

Mind backing up this statement with some links? Especially for its reasons.

If you mean the thread you started to get Object.defineProperty to ignore inheritance then its, again, modifying Object.prototype that is the cause there, not inheritance in general. Don't throw the baby out with the bathwater. Fix the right place.

And as I believe @seishun has also pointed out, unless you start doing hasOwnProperty before every object access (not just in options objects), a mangled Object.prototype is going to break code.

@jdalton:

Nice snip, now cover the second half of that sentence "and why it's worth the code/test/edge cost given the lack of common use"

  1. edge cost: Nikolai and I are saying there is no "edge cost" involved, so we can't cover that.
  2. testing cost: There is testing cost involved anyhow to make it consistent.
  3. code cost: There is code cost involved anyhow to make it consistent.

Neither testing nor code outcomes trump other in cost.

I'd even argue that removing inheritance consistently carries a far higher cost than allowing it. Every function is API area, and if you think it's reasonable to expect every function that can take a plain object to ensure it doesn't ever reference anything in that object without first stripping its parents or always using hasOwnProperty, is just a fool's errand. If this now applies only to options object, we're back to inconsistency land.

@jdalton:

@moll:

Please give us an isolated code example of the problem of taking inheritance into account in such a way where the problem is only caused by inheritance

I've already given an example of a problem directly related to inheritance of options object properties and explained the benefits of not supporting them.

Okay, I've gone through what you've said so far to summarize. While Nikolai addressed some of them already, I'll go over them quickly:

  1. Object.assign.
    One standard library function shouldn't dictate public APIs of other code.
    The language already has a concept of enumerability. It's unnecessary to now add inheritance as an indirect way of marking enumerability as well. They're orthogonal concepts.
  2. "The amount of work to treat inherited properties consistently, adding checks for Object.prototype"
    Inherited properties are treated consistently by default by all the existing language constructs. Property access goes through inheritance as does a for in loop.
    Excluding Object.prototype is only a problem if someone modifies Object.prototype, which they should never do.
  3. "Flattening inherited properties creates bizarro cases, for example when shallow cloning properties I wouldn't expect inherited properties to be flattened."
    Flattening as a mechanism of setting defaults is just one possibility. What the receiver of the object internally decides to do is up to it. It can handle his defaults without flattening for all I care.
  4. "Surprising" inherited property of Node stdlib should take inherited properties into account and do so consistently #7587 (comment)
    Answered by @seishun at Node stdlib should take inherited properties into account and do so consistently #7587 (comment).
    If there's a developer indeed surprised by this, some education might be in order. But most likely that was his intention. Then it works as expected. Let him, even if you find a better way to do so.

So, thank you for linking to your code example again, but as point 4 refutes, this is not a problem due to inheritance, it's a problem due to lack of developer education. That's almost the minimal you've got to know in JavaScript.

It seems to me you've got one case that once hurt you — someone extending Object.prototype — and now you're carpet bombing inheritance. By all means, please prevent people from mangling Object.prototype, but leave intermediate inheritances alone as they cause no extra problems in vanilla JavaScript.

@WebReflection
Copy link

this is an interesting thread that somehow summarize properly the history of JavaScript

I have been asked to pay attention and read, also I've been called in cause indirectly through some tweet or link … I am not here to take any part in this conversation, I am just here to clarify with facts the current state of JS, where it comes from, and also clarify the Object.defineProperty case.

About Object.defineProperty

The main topic of this thread is consistency but nobody reminded that Object.defineProperties accepts a second argument where only own properties will be assigned to the target.

This is inconsistent with Object.defineProperty because the latter one consider inherited too and as @jdalton said this very specific case has problems

It Is Not About Object.prototype !

What @jdalton mentioned was simply an example where undesired inheritance can cause problems or cause weakness in the env making more stuff that meant one writable.

Moreover, WeakMaps instances have get and set in their proprotype, same is for Map .

Inheriting from them, specially in the Map case, is a foot gun for any descriptor.

Inheriting from anything that by accident has one of the descriptor properties in its chain, will doom Object.defineProperty operations … do we need to start telling every dev that putting a get or set method for, let's say an Observable constructor is a bad practice because of descriptors where nobody uses inheritance from them in any case and once returned are fresh new objects regardless the initial prototype?

Really nobody seems to use, or if it does is 0.1% of the world, classes or instances that inherits from anything when they mean to defineProperty of a generic object.

As summary, this very specific case is about not having to deal with a minefield every time we use the power of ES5 and these new Object entries based on descriptors.

This case has in my opinion nothing to do with the big picture considered in this whole thread so .. moving on:

JS Has Been Advocated Through OwnProperty

This is the part where JS comes from … an era where any classical definition was able to "doom" any for/in loop which aim was to simply parse or copy properties.

An era where most famous book and the first widely adopted linter told everyone that loops without the slow and boring hasOwnProperty, a method that could be overwritten in the Object.prototype itself anyway, a method that does not even exist in Object.create(null) instances (one of the reason such linter sucks since 5 years or more), would have been somehow "disaster" prone.

We all know in this specific thread this is not true, plus we all know that thanks to new Object.defineProperty power we can define things not enumerable … regardless ...

obj.hasOwnProperty is the hottest method ever since the beginning of the time because there was no other way to understand if something was inherited or not and most JS developers read famous but a bit outdated books.

At the same time, this somehow drove every new entry in ECMAScript, most of them based on [[HasOwnProperty]]:

  1. Object.keys returns own and enumerable properties
  2. Object.getOwnPropertyNames does not even have a counterpart without the own bit
  3. Object.getOwnPropertyDescriptor does not even need a spec to understand what it does …

But back to the very specific Object.defineProperty inconsistency mentioned in this thread:

console.log(
  Object.getOwnPropertyDescriptor(
    Object.prototype,
    'toString'
  )
);

// will log something like
{ value: [Function: toString],
  writable: true,
  enumerable: false,
  configurable: true }

The problem is that defaults are returned as own properties because every developers expect that those writable, enumerable, and configurable are part of the descriptor and if not, should not be there but for readability sake, these properties are there.

Now, with a minefield or an evil attack as Object.prototype.get=function trap(){} is, this code which is perfectly valid will start failing:

var descriptor = Object.getOwnPropertyDescriptor(
  Object.prototype,
  'toString'
);
Object.prototype.get = function evil(){};
Object.defineProperty(
  Object.prototype,
  'toString',
  descriptor
);
// throws by specs!

Who's going to write such horrendous code? Surely not me, not you, nobody in this thread, not even @jdalton to make his point :P … but it's a vector, a "bomb in the system" that can compromise logic and create new flows in a code simply redirecting them through try/catch wrappers … and I don't know about you but in the Object.defineProperty very specific case, and specially for a server side oriented platform as node is, I definitively don't want to deal with this shit and sleep instead!

ES6 Entries

Object.assign is not the only new entry based on [[HasOwnProperty]], entries() and values() and all generator related instances are based on [[HasOwnProperty]] specifications … but in any case, due backward compatibility and the migration between ES versions, [[HasOwnProperty]] is still the most consitent way to deal with new and old objects across all JS engines out there.

I understand node is in a lucky position born with an almost ES5 compatible V8 engine, but the history of node is very quirky too when it comes to inheritance and problems related to Object.prototype too and null objects, just to mention one.

We Should Learn From The Past

The reason today most libraries, even the core, and most developers believe that [[HasOwnProperty]] is the only way to be safe and consistent with objects is because for many years people read about it from an era where it was not easily possible otherwise to loop over properties and/or method that where not meant to be looped.

At the same time, dogmas like the following:

Excluding Object.prototype is only a problem if someone modifies Object.prototype, which they should never do.

are the reasons today we are think that [[HasOwnProperty]] is the only way to go … not only you can modify Object.prototype without causing any problem as I've done with eddy.js, you can also use indeed the power of ES5 to enrich your own objects making things not enumerable and abandon old dogmas about the language.

Solved Anyway

I don't know what's the fuss of this whole discussion or probably I got lost somewhere … but it looks that inconsistency is the key of the initial problem … what kind of option does that method accept?

Here everything is related to what kind of object you pass to that method, one that inherited properties or one that has only own properties ?

But why I don't read anywhere that to support both the invoked method should simply check through the always fast, working, and easy in operator ?

var DEFAULT_OPTIONS = {host: "example.com"}
var opts = Object.create(DEFAULT_OPTIONS)
opts.port = 443
new Socket(opts);

// where exactly IS the problem if …
function Socket (opts) {
  if ('host' in opts) this.host = opts.host
}

In few words I don't understand why an utility to extend has been attacked instead … and to produce what, something that accordingly to the whole discussion should not matter as own properties is?

It looks to me the problem here is a dumb object check in the used constructor, not the fact an extend should consider all inherited properties … but to close and clarify about that, following my 2 cents on the overall issue.

As Summary

I really don't like loops that use obj.hasOwnProperty(key) but that's objectively the way the current web and JS land has been instructed for about 12 years or more.

If consistency is the key there is no argument against filtering by own property for the simple reason that almost everything in ES5 and ES6 will be mostly based on such assumption because developers never cared much about inheritance and inherited names.

I do, and I am not alone neither here on in es-discuss ML, but the rest of the world would like to have the least surprise specially for simple operations.

@moll
Copy link
Author

moll commented May 12, 2014

Thank you, Andrea a.k.a @WebReflection, for your thoughts and clarifications!
I understand your wish to not take part. I do feel some things deserve a comment for balance, so please bear with me.

In few words I don't understand why an utility to extend has been attacked instead … and to produce what, something that accordingly to the whole discussion should not matter as own properties is?

I apologize for that. I might've made my proposal above a little to Util._extend centric whereas my true focus was always on the API. I considered the extend function to be just one place where a lot of it can be fixed wholesale.

At the same time, dogmas like the following:

Excluding Object.prototype is only a problem if someone modifies Object.prototype, which they should never do.

are the reasons today we are think that [[HasOwnProperty]] is the only way to go … not only you can modify Object.prototype without causing any problem as I've done with eddy.js, you can also use indeed the power of ES5 to enrich your own objects making things not enumerable and abandon old dogmas about the language.

As much as I'd like for every JavaScript object to be safely extendable like you (and I can tell you anecdotes of my failures 6-7 years ago — in a controlled runtime, btw), I just don't think it's feasible. Ever. Enumerable or not. Hence the dogma — a principle from experience and reasoning.

The reason for this is plain old property access. It will probably always walk the prototypes. It's never been only about enumerability, it's been about the fact that once the core properties are no longer the only properties on your object by default, you cannot trust any simple truthy (!!obj.foo) or existence ("foo" in obj) check on any object.

And that's where it will get most of the code out there. I do remember the hasOwnProperty propaganda, but I just don't see it anywhere it matters. I remember trying to patch jQuery 6 years ago to do hasOwnProperty everywhere, but gave up, because it's simply not feasible. Every property that you cannot be 100% sure was initialized as an own property (and to do so with certainty requires Object.defineProperty) will require a hasOwnProperty check. The complexity and verbosity will get you eventually.

And so I advocate for taking inheritance into account: for consistency and less idiosyncrasies of APIs to remember. The alternative, trying to support Object.prototype modifications, is just not feasible and will never be. It's fighting with dynamic typing — late initializations. We just can't all realistically add hasOwnProperty checks everywhere, but we can agree to never change Object.prototype.

@WebReflection
Copy link

@moll I am very happy with eddy.js and it brings zero real-world problems to my plate. Don't get me wrong, I do not encourage Object.prototype modification, and I don't want to put yet another off topic in this discussion, but I abandoned all donts a while ago in favor of common sense, test driven development, and usability … eddy is already in some production and it works just fine.

Here you have different problems, consistency, and I've already said what I think it means.

eddy is, if you want, an inconsistent approach to the Object.prototype that didn't chose on purpose those method names able to ruin defineProperty intents but nobody, in the very specific defineProperty case, would expect inherited properties to be part of a descriptor, not those inherited in the Object.prototype.

Inheritance is OK as it is, without compromising too much the intent of copying properties around … I mean, isn't the inheritance purpose to share, so why copy shared properties instead of make explicit intent on what to pass as argument?

I also consider the future named function properties, where all this discussion about defaults might take another direction since inherited properties, by specs, would be less considered than own so that signature default value might occur instead.

You don't want these cases in the middle of your programming, neither do I, so hasOwnProperty it is then (still talking about consistency)

Once again, my 2 cents.

Best Regards

@moll moll changed the title Node stdlib should take inherited properties into account consistently (and thereby so should Util._extend) Node stdlib should take inherited properties into account and do so consistently May 12, 2014
@jasnell
Copy link
Member

jasnell commented Jun 22, 2015

Closing this. The conversation appears to have gone stale. Can reopen if anyone feels it needs to be.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants