es6.spec.symbols transformer: add instanceof transformation

[getify]

It seems that currently @@hasInstance symbol support isn't in place, which means that instanceof checks aren't able to be overriden. I think in the same way that typeof checks are transformed with the optional es6.spec.symbols, so should instanceof checks.

[sebmck]

I don't understand, can you elaborate on what currently happens and what you expect to happen?

$ babel-node
> Symbol() instanceof Symbol
true

[topaxi]

Aren't symbols primitives and not objects?

Firefox gives me:

> Symbol() instanceof Symbol
false
// Behaves the same as for example strings
> String() instanceof String
false

What I think @getify means is, to implement the Symbol Symbol.hasInstance, there should be an transformer for all instanceof calls.

[monsanto]

Symbol() instanceof Symbol should indeed be false, that's an (unrelated) bug

[zloirock]

@monsanto zloirock/core-js#16

[getify]

Nope, this issue has nothing to do with "symbol instanceof Symbol". That's why I mentioned the @@hasInstance WKS.

Code that illustrates:

function Foo(greeting) {
    this.greeting = greeting;
}

// override `instanceof` behavior for instances of `Foo`
Foo[Symbol.hasInstance] = function(inst) {
    return inst.greeting == "hello";
};

var a = new Foo( "hello" ),
    b = new Foo( "world" );

a instanceof Foo;           // true
b instanceof Foo;           // false

---

Edit:

The original code above has an inaccuracy. Here's the corrected section:

// override `instanceof` behavior for instances of `Foo`
Object.defineProperty(Foo,Symbol.hasInstance,{
   value: function(inst) {
      return inst.greeting == "hello";
   }
});

Symbol.hasInstance is writable: false (so can't be assigned by normal =), but it's configurable: true so it can be changed with Object.defineProperty(..) instead.

[zloirock]

@getify problem with bound functions, step 2, how do you propose implement it?

function F(){}
Object.defineProperty(F, Symbol.hasInstance, {value: _ => true});
var G = F.bind(null);
({}) instanceof F; // => true
({}) instanceof G; // => should be true

[getify]

I have not considered implementation details. Does seem like a problem at first glance. Perhaps bind(..) calls have to be wrapped, too?

Remember, I'm asking for this support in an optional transformer, so the (significant) costs of the spec compliancy around symbols only matter to those who care about spec compliancy with symbols. Alternately, perhaps this should be its own additional transformer?

[getify]

At a minimum, this needs to be documented what the support is, one way or the other. I spent over an hour combing through the docs to figure out if this was supported somewhere that I was just missing.

[sebmck]

@zloirock Could core-js copy over Symbol.hasInstance when it shims Function.prototype.bind?

@getify

I spent over an hour combing through the docs to figure out if this was supported somewhere that I was just missing.

The docs for es6.spec.symbols are pretty clear/concise.

[getify]

The docs for es6.spec.symbols are pretty clear/concise.

It took an hour to find that specific one, since I hadn't previously been familiar with the optional transformers in that level of detail. These sorts of limitations (or oversights, whatever) weren't mentioned at all in any of the FAQ's or polyfills or caveats pages, which is where I spent that time looking first.

To clarify, what I mean is that the polyfills/caveats/faq's pages could be clearer about calling out the fact that there are things which the spec calls for which aren't by default transformed, and require you to go looking for the optional transformers for such things.

[sebmck]

Yeah, the spec transformers should be mentioned in the caveats.

On Tue, Apr 28, 2015 at 2:28 PM, Kyle Simpson notifications@github.com
wrote:

The docs for es6.spec.symbols are pretty clear/concise.

It took an hour to find that specific one, since I hadn't previously been familiar with the optional transformers in that level of detail. These sorts of limitations (or oversights, whatever) weren't mentioned at all in any of the FAQ's or polyfills or caveats pages, which is where I spent that time looking first.

Reply to this email directly or view it on GitHub:
#1364 (comment)

[zloirock]

@sebmck I don't think replacing correct Function#bind in core-js is a good idea. I think, possible wrap this method only when declared this transformer.

I will add Function#@@hasInstance.

[sebmck]

It’s impossible to catch

var foo = function () {};

var bar = “bind”;

foo[bar]();

then unless you wrap all member expressions too. How far is too far?

On Tue, Apr 28, 2015 at 3:05 PM, Denis Pushkarev notifications@github.com
wrote:

@sebmck I don't think replacing correct Function#bind in core-js is a good idea. I think, possible wrap this method only when declared this transformer.

I will add Function#@@hasInstance.

Reply to this email directly or view it on GitHub:
#1364 (comment)

[zloirock]

@sebmck I mean add with instanceof helper, not for bind call, something like

if(!Function.prototype.bind.__wrappedForHasInstance__)
  var _bind = Function.prototype.bind;
  Function.prototype.bind = function(){
    var result = _bind.apply(this, arguments);
    if(Symbol.hasInstance in this)Object.defineProperty(result, Symbol.hasInstance, {
      enumerable:   true,
      writable:     true,
      configurable: true,
      value:        this[Symbol.hasInstance]
    });
    return result;
  }
  Function.prototype.bind.__wrappedForHasInstance__ = true;
}

It's better then slowdown w/o this optional transformer.

[sebmck]

As in, add that to the top of every file?

[zloirock]

Together with instanceof helper.

[sebmck]

The instanceof helper is going to be included in the external helpers build so best not to pollute globals. Also doesn't stop bind before the instanceof helper (and thus before Function.prototype.bind pollution) being loaded.

[zloirock]

Anyway, wrapper for correct Function#bind in core-js is a bad idea.

[sebmck]

Just a suggestion.

[getify]

[Instead of opening a new issue, I'm going to post this question here, so please reopen. Am also happy to spin off a new issue thread if more appropriate.]

I appreciate the support added here for transforming instanceof checks so that overrides of @@hasInstance work. However, there are several other well-known symbols that extend native behavior, and it seems like we should disposition support for each one of them.

Some of them are probably possible to do similar to the @@hasInstance change, while others of them seem perhaps completely impractical to handle. So maybe we should discuss each one and either add the support or update the caveat docs (I'm happy to do so with a PR) to indicate that support is not possible.

Here are the Well Known Symbols:

• @@iterator: supported
• @@toStringTag: supported
• @@hasInstance: supported
• @@unscopables: filters out properties fromwith, which doesn't work in strict mode
• @@species: on classes derived from built-ins (like Array), controls how new instances are created by instance methods (like slice(..)). babel test
• @@toPrimitive: controls how objects are coerced to a primitive value when used with operators like +. babel test
• @@isConcatSpreadable: controls if Array.prototype.concat(..) spreads out a particular value or leaves it alone and just includes it as-is. babel test
• @@match, @@replace, @@seach, and @@split: controls how regular expressions are matched when used with the String.prototype.* methods of the same names. babel test

So, from this list, specifically the ones that are not already supported, is it feasible to support any of them?

• Seems like @@isConcatSpreadable could be fairly easy, by wrapping Array.prototype.concat(..).
• @@match and friends could be done by wrapping all the built-in prototype methods of both RegExp and String.
• @@toPrimitive seems impractical, since it would require wrapping pretty much every operator.
• @@species might be possible, but there's already (I think?) caveats where extending natives isn't fully supported, so maybe not?
• @@unscopables only matters in non-strict mode, so seems like it's out.

Thoughts?

[zloirock]

@getify I don't wanna add @@isConcatSpreadable and @@species for some methods - performance. @@match and friends - maybe, I have long thinking about it. BTW, it's core-js, not babel, issue.

[sebmck]

@getify toPrimitive is the only relevant one to Babel and it's impractical, because as you said, require wrapping all operators which is goes against how most people use Babel which is for production code.

[getify]

OK, I figured that would be the conclusion. Should I submit a PR to the babel site repo that adds such to the "caveats" page?

[sebmck]

Sure, would be appreciated!

On Wed, Apr 29, 2015 at 2:43 PM, Kyle Simpson notifications@github.com
wrote:

OK, I figured that would be the conclusion. Should I submit a PR to the babel site repo that adds such to the "caveats" page?

Reply to this email directly or view it on GitHub:
#1364 (comment)