[Bug fix] @autobind and @mixin decorators should be able to handle ES6 Symbols

[kkwiatkowski]

Greets,

Writing my own project I found, it is not possible to use autobind and mixin decorators with Symbols beside. This should works out of the box, since whole logic is already done in core-decorators' internals, but due to not enumerable nature of Symbols, it is not possible to iterate over them using for in construction (Object.keys() also doesn't work for those). Both, mixin and autobindshouldn't really care if something is enumerable or not.

Cheers!

[kkwiatkowski]

By they way. I found why build for node 0.10 doesn't work. I know that it is easy to say, but for this particular case, it is internal bug of node.

As you can see this test doesn't pass, but check the message:

  1) @mixin correctly adds symbols:
     TypeError: Property 'Symbol(mixin)_k.208j4grh3bdtpgb9' of object [object Object] is not a function
      at Context.<anonymous> (test/unit/mixin.spec.js:75:20)

After some investigation I've found that node is going crazy when you try to use in operator to check, if some property is already somewhere in prototype chain of given object.

Here is an example:

const hash = Symbol('hash');
function A() {}
A.prototype = {};

console.log('Node goes crazy? ', hash in A.prototype ? 'YES' : 'NO'); // -> 'Node goes crazy? YES'

class B {}

console.log('Node goes crazy? ', hash in B.prototype ? 'YES' : 'NO'); // -> 'Node goes crazy? YES'

Eventually, the final answer, why does it matter?

function handleClass(target, mixins) {
  ...
    for (const [key, desc] of descs) {
      if (!(key in target.prototype)) { // <-- viola, this is the case...
        defineProperty(target.prototype, key, desc);
      }
    }
  ...
}

What do you think about it? Should I add test to check if node is buggy just before test case, or should I traverse for the whole prototype chain of mixin target to check on every level if there is such symbol key?

[kkwiatkowski]

Using this test case:

  it('correctly adds symbols', function () {
    const symbolHash = Symbol('mixin');
    const SymbolMixin = {
      [symbolHash]() {
        return 'symbolHash';
      }
    };
    class A {}
    class B extends A {}
    @mixin(SymbolMixin) class F extends B {} // extends are only to show the issue
    const foo = new F();

    foo[symbolHash]().should.equal('symbolHash');
  });

And this test code:

// src/mixin.js
function handleClass(target, mixins) {
  ...
    if (!(hasProperty(key, target.prototype))) {
  ...
}

const isNodeBuggy = function() {
  const foo = Symbol('foo');
  class Foo {}
  return foo in Foo.prototype
}();

const {getOwnPropertySymbols, getPrototypeOf} = Object;

function hasProperty(prop, obj) {
  // Node in version 0.10 treats type of Symbol as "object" instead of "symbol".
  const type = typeof(prop);
  if (isNodeBuggy && (type === 'symbol' || type === 'object')) {
    do {
      console.log('Constructor: ', obj.constructor.name);
      console.log('Own symbols: \n', getOwnPropertySymbols(obj));
      console.log('Needed symbol: ', prop);
      console.log('Has one: ', obj.hasOwnProperty(prop));
      console.log('Is this Object.prototype: ', obj === Object.prototype);
      console.log('Going down the chain...');
      console.log('\n');
      if (obj === Object.prototype) {
        return false; // If we arrived to the last prototype, we can be sure
                      // that there is no needed symbol in chain...
                      // Because due to the node.js bug, every symbol is stored
                      // in Object.prototype
      }
      if (obj.hasOwnProperty(prop)) {
        return true;
      }
    } while (obj = getPrototypeOf(obj));
    return false;
  } else {
    return prop in obj;
  }
}

We will receive such results:

Constructor:  F
Own symbols: 
 []
Needed symbol:  { _k: 'Symbol(mixin)_l.z1hvj9oxhn34bo6r' }
Has one:  false
Is this Object.prototype:  false
Going down the chain...

Constructor:  B
Own symbols: 
 []
Needed symbol:  { _k: 'Symbol(mixin)_l.z1hvj9oxhn34bo6r' }
Has one:  false
Is this Object.prototype:  false
Going down the chain...

Constructor:  A
Own symbols: 
 []
Needed symbol:  { _k: 'Symbol(mixin)_l.z1hvj9oxhn34bo6r' }
Has one:  false
Is this Object.prototype:  false
Going down the chain...

Constructor:  Object
Own symbols: 
 [ { _k: 'Symbol()_f.z1hvj9oxhn34bo6r' },
  { _k: 'Symbol(__core_decorators__)_i.z1hvj9oxhn34bo6r' },
  { _k: 'Symbol(foo)_j.z1hvj9oxhn34bo6r' },
  { _k: 'Symbol(park)_k.z1hvj9oxhn34bo6r' },
  { _k: 'Symbol(mixin)_l.z1hvj9oxhn34bo6r' } ]
Needed symbol:  { _k: 'Symbol(mixin)_l.z1hvj9oxhn34bo6r' }
Has one:  true
Is this Object.prototype:  true

So @jayphelps, the question is if we should add fallback support for node in version 0.10 as shown above?

[jayphelps]

I'm confused why we would hit this anymore since this PR moves away from for..in to for..of?

[kkwiatkowski]

We don't really have to hit it anymore. I've only noticed, that there is build check for node.js in version 0.10 which failed, so I thought that support for this particular version is somehow important. Also, tests that fails every build check is not something to expect, and that's why I'm asking you:

1. If I should add exception in that test case when bug is available,
2. or just delete that test case,
3. or just add fallback support that makes it work on older node version,
4. or just remove build checks for node in version 0.10.

[jayphelps]

It's only testing 0.10 because I've haven't updated it in forever. It's never failed with this issue before, so that's why I'm confused; if it started failing in this PR because of adding symbol support + for..in, but now you've switched to for..of, I'm not seeing why it should continue to fail/be an issue?

[kkwiatkowski]

It's failing because I've added test case for mixing symbols. It wasn't tested before, thus it didn't fail. And because of that test case failing I found, there is a bug in node.js version 0.10 which is responsible for that.

To be more specific about that node bug:

1. Node is treating Symbol type as "object" instead of "symbol" which is not met with documentation.
2. When you declare Symbol like const hash = Symbol('hash');, it will be added automatically (and magically) to Object's prototype (although it shouldn't).
3. Because every constructor/class in JavaScript inherits prototype of Object (see point 2) by default, then automatically every instance of object-derived construction like: class Foo {} will have access to Symbols in Object.prototype (see point 2).
4. Because of point 3, checks like key in target.prototype (which is for example here) will be always true, because in operator traverses prototype chain until it reaches Object.prototype.
5. Because of point 4, and in relation to this, mixin decorator will never ever mix in any Symbol to target prototype in node.js version 0.10.
6. Implication of point 5 is that test case is failing because it cannot find function under symbol hash property (undefined is returned instead of function), thus build check fails:

  1) @mixin correctly adds symbols:
     TypeError: Property 'Symbol(mixin)_k.7jz70naz5yfa8aor' of object [object Object] is not a function
      at Context.<anonymous> (test/unit/mixin.spec.js:75:20)

[kkwiatkowski]

I made more detailed investigation, and it turned out node in version 0.10 does not support ES6 Symbols at all. It means, described above bug is related to ES6 Symbol polyfill provided by Babel 5. I've added support for such edge cases in latest commit. Now, decorators are working fine, also, build checks are not failing :+1:.

[kkwiatkowski]

I've created an issue on babel's bug tracker about that symbol bug, and received an answer that they unfortunately will not fix it. So I think it's good idea to have support for it just in case. Here is a link to that issue: https://phabricator.babeljs.io/T7357

[jayphelps]

Thanks for all your hard work!!

[kkwiatkowski]

Feel welcome and thanks to you too for fast responses and involvement.

[kkwiatkowski]

Do you have any idea when there will be release with this fix? Is there any release procedure?

[jayphelps]

@kkwiatkowski Yeah, the official procedure is to bug me if I forget haha whoops!

Published just now as v0.12.3.

[machard]

on React Native android it causes a red screen related to Symbol. see https://github.com/yelouafi/redux-saga/issues/54