paulmillr / es6-shim

ECMAScript 6 compatibility shims for legacy JS engines
http://paulmillr.com
MIT License
3.11k stars 389 forks source link

Add things to es6-sham #227

Open ljharb opened 10 years ago

ljharb commented 10 years ago

We need to create an es6-sham file. Anything that doesn't 100% match the spec in either an ES3/shimmed ES5, or true ES5 environment does not belong in es6-shim and should go in the sham file instead.

cscott commented 10 years ago

Repeating what I said in #225, I don't think it's appropriate to move Object.setPrototypeOf into sham, because on IE < 11 there's no possible way to make it work (so no sham), and on all other browsers it works fine. Opera 12 is the only case where there's a bug that makes our implementation slightly dodgy because we have to workaround a bug in their __proto__ setter (and not really that bad), but if forced to choose I'd rather just drop support for Opera 12 entirely since it's an abandoned codebase with a tiny user share.

OTOH, the es6 spec guys are a bit ambivalent about Object.setPrototypeOf in any case; see http://esdiscuss.org/topic/mutable-proto#content-5 where it was decided that it was a mistake due to some concerns about "realms" and secure execution. But it was put back in the standard after that point (I can't find the reason in the archives). So if we drop setPrototypeOf and force people to use __proto__ instead, it won't be the end of the world. As the ES6 discussion says, __proto__ is the defacto standard supported by every modern browser. The only difference between __proto__ and Object.setPrototypeOf are details related to inheritance and realm support which probably are moot in an ES5 engine anyway. (I think "objects which don't inherit from Object.prototype" don't get Object.prototype.__proto__ and so you need to use Object.setPrototypeOf in order to change their prototype.)

WebReflection commented 10 years ago

I have exactly same fear that developers will think it's then OK to use __proto__ so there won't be concretely any benefit in dropping Object.setPrototypeOf here.

TC39 agreed on Object.setPrototypeOf and actually moved __proto__ to annex B somehow discouraging its use.

@rwaldron confirmed it was the right decision/direction plus __proto__ suffers problems developers easily fall into, like the inability with {__proto__:null} object to be promoted via __proto__ causing easily shenanigans in the logic.

So, just to be pragmatic: Object.setPrototypeOf is standard, adopted natively already in IE11 and Chrome, others will follow.

If es6-shim would like to promote the usage of something potentially problematic as __proto__ is that indeed required changes that gave birth to a better Object.setPrototypeOf, at least write all problems related to the usage of __proto__, same as it you have done in es5 for WeakMaps

Then in both cases it's about my contribution so I do hope there is nothing personal behind all this pushing back.

I will in any case approve any decision you believe is good for the community since I am very tired about __proto__, discussions, sorry.

Best Regards

cscott commented 10 years ago

@WebReflection is right, currently __proto__ is in appendix B of the ES6 spec. So we shouldn't really be encouraging node authors to use it (as one example of a non-appendix-B JS implementation -- although node implements __proto__ currently). For future-proofing, node authors should be using Object.getPrototypeOf and Object.setPrototypeOf.

In my understanding, "sham" is used for implementations that are "close but not quite according to the spec, likely in surprising ways". For example, weak maps that aren't weak at all. setPrototypeOf isn't a sham, it's just absent on IE<11.

WebReflection commented 10 years ago

as extra note, in node __proto__ is not behaving as expected concretely across versions.

Again, there are older V8 accepting __proto__ in objects created via Object.create(null) or {__proto__:null} while this should never happen with null objects/dictionaries … Object.setPrototypeOf at least tries to guarantee as much consistency as possible through an explicit intent and it's safer with dictionaries as generic obj[key] access, widely adopted in both client and server, rarely guarded being trusted as dictionaries or null objects.

These are just my last 2 cents

WebReflection commented 10 years ago

P.S. I've said Chrome, well, at least Object.setPrototypeOf is already in Canary and latest builds of V8 so Chrome will follow (if it hasn't done it already). Just as extra clarification. Take care

rwaldron commented 10 years ago

Just want to confirm that Object.setPrototypeOfis definitely in ES6.

@cscott node doesn't implement __proto__, it exists in node because it exists in v8.

...And the es-discuss thread you cited is out of date. The final decision to add the API was in May of 2013, and the API has been included in the ES6 specification for 9 months.

Relevant: https://bugzilla.mozilla.org/show_bug.cgi?id=885788 (in addition to @WebReflection's report for Canary)

ljharb commented 10 years ago

Thanks all for the clarifications on __proto__ versus setPrototypeOf. I'm in full agreement with @WebReflection etc that we should be encouraging usage of setPrototypeOf.

With respect to the original issue, @cscott's point is the most relevant imo - that the implementation will simply be absent, but never present and incomplete, in some browsers. This convinces me that it should remain in the shim, although of course we should do our best to make it be both present and complete on as many engines as possible.

I'll edit the description to cross out setPrototypeOf, but leave the issue open to cover anything else besides WeakMap that might be a good fit for a sham lib.

cscott commented 10 years ago

@rwaldron Yes, I meant that node currently implements __proto__ even though it's an "Annex B" feature ("The content of this annex is normative but optional if the ECMAScript host is not a web browser"). Since node is not a web browser, one could imagine that a future version of node (and a future version of v8) might allow node to drop some of these features. I have no particular opinion or insight as to whether they are likely to do that; certainly web compatibility comes "for free" with v8 and is more useful than not.

Thank you for providing a citation for the inclusion of Object.setPrototypeOf. I knew that the previous decision had been altered, given the current state of the spec, but I couldn't find the links you provided to explain why.

It seems like everyone is agreed that authors should use Object.get/setPrototypeOf in new code.

Our shims might not work correctly for the Object.setPrototypeOf(Object.create(null), ...) case, although it appears that Firefox and Safari already can handle this in @WebReflection's shim by extracting the setter, and old versions of v8 probably support this by including __proto__ even on objects created via Object.create(null). This is the final gotcha which might bite users of the shim. Any idea how widespread it is? (I should probably add a test case, but with testling down it would be hard to get reasonably broad cross-browser results.)

cscott commented 10 years ago

@ljharb there's already bug #57; we should probably continue to use that bug for sham-related discussion.

cscott commented 10 years ago

Hm. Object.setPrototypeOf(Object.create(null), ...) doesn't work on node 0.11 and Chrome 33. See https://github.com/cscott/es6-shim/commit/spo-test and test results at https://travis-ci.org/cscott/es6-shim/builds/19834337 (and https://ci.testling.com/cscott/es6-shim but that appears to still be broken).

@WebReflection, @rwaldron is there a workaround for this? Now I'm starting to think our implementation is a sham after all... but maybe this is an unimportant corner case?

WebReflection commented 10 years ago

@cscott the only abused example I have is the usage of Dict

var Dict = Object.create.bind(null, null);

var o = Dict();
o.__proto__; // will it blend?

If that is everything but undefined then users are risking any sort of problem with generic o[key] access where key is not guarded against "__proto__" dropping inevitably one word form that dictionary.

Hopefully this is a minor security concern that does not affect IE polyfilled behind Object.create, but affect some quite old Android and/or node.js version (it was a V8 problem)

Everything else, well, except now some Opera 12 apparently, should work without problems if they have __proto__, won't work if they don't.

In the latter case, the solution is actually to always readdress the promoted object so that Internet Explorer can do black magic behind the scene.

var a = {}, b = {};
// instead of this
Object.setPrototypeOf(a, b);

// try to always do this
a = Object.setPrototypeOf(a, b);

// so that
b.isPrototypeOf(a);

// and IE < 11 could eventually do
if (!Object.setPrototypeOf) {
  Object.setPrototypeOf = (function(has){
    function Chain(o) {
      for (var k in o) {
        if (has.call(o, k)) {
          this[k] = o[k];
        }
      }
      Chain.prototype = null;
    }
    return function(o, p) {
      Chain.prototype = p;
      return new Chain(o);
    };
  }({}.hasOwnProperty));
}

// loosing reference to initial object
// but hey … you can't have everything!

Thanks Rick for the more than valuable comment.

Best Regards

WebReflection commented 10 years ago

@cscott in comments:

setPrototypeOf.polyfill === false means it's not 100% reliable

node 0.11 is using a version that does not expose __proto__ but it does not accept it neither for Object.create(null) cases, these cannot be promoted … quite rare if you ask me, more rare than using WeakMap ;-)

I believe you should change tests, if(Object.setPrototypeOf.polyfill === false) … warn or notify.

comments were already in the code

These are reason it's hard to put a line between shim and sham for this case but 0.11 is also not stable so I hope V8 will be updated in both Chrome and node soon since Canary already supports it natively.

WebReflection commented 10 years ago

P.S. as last reminder … all these problems with __proto__ are the reason Object.setPrototypeOf has been proposed and accepted.

WebReflection commented 10 years ago

@cscott sorry for bothering once again … last thought:

When anyone uses var o = {__proto__:null} they do not expect to use later on o.__proto__ = Array.prototype because by specifications objects with a null prototype do not inherit the __proto__ behavior (Dictionaries).

Accordingly, whoever is using __proto__ instead of Object.setPrototypeOf will never be affected by this Chrome 33 or node 0.11 problem because they won't expect to promote such kind of object.

Generally speaking, I think is a good technique to not promote dictionaries since these are not meant to be instanceof Class since born so I would say this Object.setPrototypeOf is an almost complete shim that promote a better approach to hot-swapped inheritance.

Really nothing on the real world came up more to justify this method as part of es6-shim.

Hopefully last thoughts :-)

WebReflection commented 10 years ago

FYI: node.js v0.11.13-pre includes native Object.setPrototypeOf

WebReflection commented 10 years ago

FYI: Java 8 nashorn ships without __proto__ but it implements Object.setPrototypeOf(O,p) … as you can see, it's rather __proto__ finally disappearing from the scene, instead of Object.setPrototypeOf(O,p) being a sham ;-) I would personally close this task as invalid - Best Regards

ljharb commented 10 years ago

It belongs in a sham, not the shim, if it can't be 100% implemented in every ES3, or ES5, browser or JS engine. It's wonderful news that more browsers are removing __proto__, but we're stuck with shimming all the existing browsers too.

WebReflection commented 10 years ago

nothing can be implemented 100% in ES3 … Array.prototype is enumerable, as example, in there so es5-shim should be 90% sham … does this make any sense? I hope no

ljharb commented 10 years ago

That's why I said "ES3, or ES5". You'll note that every function in the ES6 shim works 100% in ES5, and the ones that can't be 100% implemented in ES3 are explicitly skipped over. To qualify for this shim, a polyfill must work in a true ES5 browser at a minimum, and ideally, in an ES3 one as well.

WebReflection commented 10 years ago

and it does work in ES5 browsers … what are you trying to say?

ljharb commented 10 years ago

281 created an es6-sham file.

kaleb commented 9 years ago

One of the only use-cases that I have for using Object.setPrototypeOf is to make callable instances. Take this example:

function Prop(value) {
    function prop(newValue) {
        if(!arguments.length) return value;
        value = newValue;
        return this;
    }
    return Object.setPrototypeOf(prop, Prop.prototype);;
}
Prop.prototype = Object.create(Function.prototype);
Prop.prototype.toJSON = function() {
    return this();
}

var myProp = Prop('test');
console.assert(myProp() == myProp.toJSON(), 'values should be the same');

The setPrototypeOf sham does not work for this in IE <= 10. Would anybody have any objections to use Object.assign as a last resort to copy methods from the inherited proto to the new proto?

Edit for clarification

The reason why this is not working is that the Object.setPrototypeOf is returning an object instead of the intended function.

WebReflection commented 9 years ago

@kaleb you are addressing, setPrototypeOf in IE < 9 must create an instanceof the prototype you are passing, nothing that Object.assign could solve.

The sham simply use Object.create(proto) where this will never be invokable by specs so I think you should revisit your pattern for IE < 9.

Your pattern also breaks in IE < 9 because it won't respect use strict and the return this inside the prop function will return the global window object … moreover it's not clear what that this should point at, the prop function or anything that might be passed as context ?

Long story short, I'd personally simplify your pattern, also considering that never worked for 15 years in JS I am not fully sure you do need invokables done in that way.

Just my 2 cents

P.S. forget my note on inheriting Function.prototype … but you actually return a function you really don't need to do Prop.prototype = Object.create(Function.prototype);, IMO

ljharb commented 9 years ago

I'm very surprised at the notion that you can subclass functions and have something callable - I was pretty sure that ES6 allowed subclassing of Arrays, but nothing can be invoked except a function or a generator.

kaleb commented 9 years ago

@ljharb You cannot subclass Functions and have them callable. But you can set the prototype of a function so that it may have inherited properties on it. I understand that my suggestion would make the instanceof operator not work. This method of making callable methods does work in Chrome and IE11. Previously before Chrome added Object.setPrototypeOf, I would just assign __proto__ in the browsers that supported it and would do the equivalent of Object.assign in those that don't. In my example, I could remove the Prop.prototype = Object.create(Function.prototype); line, and it would work as I would like except that Function.prototype.call and the like would not consistently be available.

kaleb commented 9 years ago

A simplified example of adding a prototype to a function:

function foo(){}
var proto = {
    bar: function(){}
}
Object.setPrototypeOf(foo, proto);
foo.bar()

D3 is one library that sets the prototype of functions: https://github.com/mbostock/d3/blob/master/src/core/subclass.js

Knockout is another: https://github.com/knockout/knockout/blob/c6cdb3ead3221cf1bb4de7da2a5ed3c4e1f60162/src/utils.js#L152

My intention of commenting on this issue was to document that the Object.setPrototypeOf sham does not work with functions and also to ask if supporting this edge case is in question. Both d3 and ko use an Object.assign equivalent if they are not to set the prototype.

I do understand that this would make instanceof not work, but instanceof does not consistently in JavaScript anyway, e.g. calling instanceof on an object derived from another context (like from an iframe). If I had to choose between instanceof or having access to my methods, I would prefer the latter.

edit

It looks like d3 uses __proto__ or extend for arrays, not functions. It must've been some plugin that I was using that extended functions similarly. Neverless, it is a useful pattern already available in browsers that support either __proto__ or setPrototypeOf without sh[ai]ms.

WebReflection commented 9 years ago

instanceof was a shortcut to say that inheritance should work as such. If you want callable instances with attached methods you can already do that by your own.

Being a sham means there are caveats and this is one of them for all engines that cannot swap inheritance at runtime.

You are going to have problems with .call anyway since your invokable object won't have Function.prototype in its inheritance anymore … this would break everything that trust typeof obj === "function" which is a way more common pattern than yours.

I don't see any advantage in having invokable objects, you can always set that as property that could be self bound, as example, without going too fancy with instances that behave unexpectedly weirdly behind common ( superficial ) type checks and analysis you could have.

Once again, please consider this:

var f = Object.setPrototypeOf(function () {}, {});
f instanceof Function; // false
f.call; // undefined
typeof f; // function

The last one won't fail cross realm and it says nothing about the hybrid status of the object.

Moreover, setPrototypeOf is quite explicit, having f not inheriting from the prototype you pass will make it completely pointless plus there are properties that cannot be overwritten such name which is a very common property for generic objects.

Using assign as lat resource after Prop.prototype = Object.create(Function.prototype) will be pointless too since properties will be non enumerable or behind getters/setters but basically all I believe you should simply do is the following:

function Prop(value) {
    function prop(newValue) {
        if(!arguments.length) return value;
        value = newValue;
        return prop;
    }
    return Prop.create(prop, Prop.prototype);
}

Prop.create = '__proto__' in Prop ?
  // IE11 and all others
  Object.setPrototypeOf :
  function (fn, proto) {
    for (var key in proto) fn[key] = proto[key];
    return fn;
  }
;

Prop.prototype.toJSON = function() {
    return this();
}

About this pattern

I am not sure what d3 truly gained using this pattern, it's slower per each object creation and more confusing, IMO.

If the private value was the goal, I think there are better approaches where obj.value(newVal) and obj.value() would not look that bad and it will be implemented in the prototype so that all your inheritance problems are solved.

Here an example


function Prop(value) {
  return this instanceof Prop ?
    this.value(value) : new Prop(value);
}

// you can use Object.defineProperty
// if you care about enumerability
Prop.prototype.value = function (value) {
  var self = this;
  self.value = function (newValue) {
    if(!arguments.length) return value;
    value = newValue;
    return self;
  };
};

// plus more magic!
Prop.prototype.toJSON = 
Prop.prototype.valueOf = function () {
  return this.value();
};

So, now you can:

// create an instanceof Prop
var p = new Prop(123);

// that will have the expected value
p.value(); // 123

// it can update it
p.value(456);

// and it can also be compared
if (p == 456) console.log('yeah');

Hope you'll revisit this pattern, but I also hope you understand why asking fallbacks that might act unexpectedly is not a good pattern for a sham ora polyfill in general.

Cheers

kaleb commented 9 years ago

@WebReflection Thanks for your thoughtful reply. I already use the valueOf and toString magic. Thanks for mentioning it, though.

What is the appropriate way to tell if Object.setPrototypeOf is a sham?