updated lang.isObject to work in ES6 Fixes #62

[devpaul]

Updated and ran tests against Chrome 44. Tests pass. Coverage reports it was run 25 time, although it's not directly called in tests. Related to #45 Fixes #62

[devpaul]

Looking at #45 it might be desirable to exclude array and regexp. Let me know if we want to add this exclusion and I can add

!Array.isArray(obj) && !(obj instanceof RegExp)

However, it's worth noting that all the tests passed, so we can't be too concerned with it, eh? ;)

[jason0x43]

Probably all host objects (RegExp, Date, Promise, ...) should be excluded, since in those aren't things you'd want to copy by simply traversing properties. Also, arrays are not "objects" in this use case.

[kfranqueiro]

Yes... the isObject function isn't about "is this any sort of object", it's about "is this an object we actually want to recurse into for deep copying" (and arrays are already handled separately by the logic in lang). So I don't think this PR has the right idea... it's just going full-circle back to the same exact problem we had in #45.

The intent of fixing that was to come up with a simpler and less brittle solution that we wouldn't just keep playing whack-a-mole with things to exclude, but #62 points out that the current solution still has a caveat.

[devpaul]

First, let's rename this method shouldDeepCopy() so as to be clear of its purpose.

Starting in ES6 using toString to identify the type as an plain object is no longer an option. The spec states this explicitly:

ES6 Object.prototype.toString

Historically, this function was occasionally used to access the string value of the [[Class]] internal slot that was used in previous editions of this specification as a nominal type tag for various built-in objects. The above definition of toString preserves compatibility for legacy code that uses toString as a test for those specific kinds of built-in objects. It does not provide a reliable type testing mechanism for other kinds of built-in or program defined objects. In addition, programs can use @@toStringTag in ways that will invalidate the reliability of such legacy type tests.

We can detect if a Symbol has been used:

var a = [];
a[Symbol.toStringTag] = "Object";
Object.prototype.toString.call(a);   /// "[object Object]"

// Direct identification of StringTag fiddling
typeof a[Symbol.toStringTag] === 'undefined'  /// false

// Another means of identification
var syms = Object.getOwnPropertySymbols(r); /// [Symbol(Symbol.toStringTag)]
syms.some(function(sym) { 
    return sym === Symbol.toStringTag;
}); /// True

But we can no longer use toString as a reliable means of testing. And AFAIK there is not a way to get the original toStringTag of a value similar to how one may use Object.prototype or Array.prototype to get at the root methods of a prototype chain.

I've been through the ES6 spec and haven't found a better way than checking if something is an object and then explicitly blacklisting objects I would not want to copy.

[devpaul]

Ugh, I almost hate that I found this. The spec states: If Type(tag) is not String, let tag be builtinTag. So we can return an object to ES5 functionality by setting toStringTag to a non-string:

/// Given
Object.prototype[Symbol.toStringTag] = 'Surely not an object'
var r = {};
Object.prototype.toString.call(r)  /// "[object Surely not an array]"
r[Symbol.toStringTag] = 'Array';  /// for lolz
Object.prototype.toString.call(r)  /// "[object Array]"

/// Then
function shouldDeepCopy(obj) {
    var oldTag;
    if (typeof r[Symbol.toStringTag] === 'string') {
        oldTag = r[Symbol.toStringTag]
        r[Symbol.toStringTag] = undefined
    }
    var shouldDeepCopy = Object.prototype.toString.call(r)  ///"[object Object]"
    if (oldTag) {
        r[Symbol.toStringTag] = oldTag;
    }
    return shouldDeepCopy;
}

This requires briefly modifying the original object meaning that if they do Object.frozen(original) then I have no idea what we would do. AFAIK we can't unfreeze a frozen object. I guess we could try get an instance of the constructor or walk up prototype chain until we find a segment in the chain where toStringTag is undefined and if it is not and everything is frozen all the way to Object.prototype, attempt to create a new instance at some level of the prototype chain and set its Symbol.toStringTab = undefined?!

If we want to use the method above to address the Symbol.toStringTab corner case we need to find a work-around for a frozen object. Maybe there's an obvious way around this but I've been looking at the ES6 spec long enough that my brain is frozen.

I still vote to explicitly blacklist objects we don't want to copy.

[kitsonk]

Bleurgh... Should we drop lang.isObject() from the public API? I hate saying "well actually we only intend to use it for deep copying". If we have no other valid use case, then we should just not expose it and then take the shortest path for detecting just what we need. I know at one point we were going to try to avoid these "artificial" detections in Dojo 2 because for every rule there are 100 exceptions (which seems to still be true).

[kfranqueiro]

It is not (and never was) in lang's public API in Dojo 2, and is not exposed (which is why I referred to it as "lang's internal isObject function" in #45; others have loosely referred to it as lang.isObject which makes it sound like it is exposed, but it's not).

My concern has always been 100% related to making it do exactly what we need for the deep assign/mixin functions, but preferably in a way that isn't going to cause us to play a decade-long game of whack-a-mole. That's why I created #45, and we had thought we'd resolved that... until Bryan pointed out #62 (thanks, ES6!).

I agree with Paul's idea of renaming the function.

[kitsonk]

@kfranqueiro oops... yes, sorry, just noticed that... 😢 and agree with renaming

[kitsonk]

Where are we on this PR?

[kfranqueiro]

Looks like the function hasn't been renamed yet (Paul suggested shouldDeepCopy; perhaps shouldDeepCopyObject is even clearer given that arrays already have their own code path).

More importantly, the implementation on this branch currently doesn't filter what we need it to, either (e.g. Date objects would return true, but should not be deep copied).

Really, the existing implementation was fine AFAIK except for the Symbol caveat. :/

[kitsonk]

Picking this up... The convention of guards is is... so unless anyone morally objects, we shall name it isObjectCoercible.

[kfranqueiro]

In reviewing this with Mangala again, I am very much in favor of a name such as shouldDeepCopy rather than isObjectCoercible - the former very clearly indicates why this function is called, and may help to avoid further misunderstandings/regressions such as what ended up happening with this PR. Our style guide says that is, has, can, and should are all fair game for things that return booleans.