paulmillr / es6-shim

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

Number constructor issues #365

Closed zloirock closed 8 years ago

zloirock commented 8 years ago
  1. [critical] IIRC it was already mentioned. Redefinition Number constructor uses Object.getOwnPropertyNames. It can't be completely polyfilled. Missed fallback for ES3 environment. What does it mean? All ES5- constructor properties lost in IE8-. Number.NaN, Number.MAX_VALUE, Number.NEGATIVE_INFINITY, etc...
  2. this instanceof Number in constructor not enough for new detection. For examplle, issue about Chai. In es6-shim some subsets of this bug, 2 main:
    • In enough modern browsers w/o support octal / binary - typeof Object(1).constructor(2) == 'object'
    • [critical] In IE9- +new Number(1) throws error
  3. Compatibility with Meteor - cant be fixed with current es6-shim arhitecture. Not a bug, just for info.

Also was some issues about _.isNative(Number).

ljharb commented 8 years ago

_.isNative is a hack, so I'm not interested in nor concerned with supporting it.

1) Good call! This indeed is using a non-shimmable method. I'll fix this ASAP.

2) Thanks for the detailed examples - I'll add a bunch of test cases for that, as well as testing in IE 9.

3) I commented why the Meteor issue is invalid - checking instanceof or constructor isn't ever correct anyways because of cross-realm issues.

ljharb commented 8 years ago

Turns out the "wrap constructor" l implemented doesn't work in ES3 - I just forgot to check for true ES5 support before adding that particular shim. I'll add the guard, and update the docs.

+new Number(1) isn't throwing an error for me in IE 9 - can you test with the latest version of es6-shim and confirm? a jsfiddle that repros it would be ideal.

typeof Object(1).constructor(2) indeed should be number. However, it does not seem possible, without new.target, to detect new usage without instanceof. I'll note it as a caveat, but I'm not too concerned since nobody actually writes production code that does that :-)

zloirock commented 8 years ago

+new Number(1) isn't throwing an error for me in IE 9

Oops, IE9 was in quirks mode, it's IE8- NFE bug. default

IE9 specific bug - typeof 1..constructor(2) == 'object' - strict mode.

I just forgot to check for true ES5 support before adding that particular shim.

Strange requirement - all properties not accessors.

However, it does not seem possible

Possible and without any serious problems :)

ljharb commented 8 years ago

@zloirock how can you detect "new" between Foo.call(Foo) and new Foo() without new.target?

zloirock commented 8 years ago

@ljharb abstract Foo? .bind-based way. But here Foo is Number wrapper. How can you detect is this number object or Object.create(Number.prototype)? :) You can just see core-js implementation instead of those questions.

ljharb commented 8 years ago

I'm afraid I can't really make any sense of your code, so I can't just "see" it. If you're unwilling to share it here in a specific manner, then that's fine. While I see that you're testing that Number#valueOf fails, how would you guarantee that Number.call(Object(2), 2) should return a primitive rather than an object?

zloirock commented 8 years ago

Instead of new on the wrapper, in this case, Number#valueOf not fails.

ljharb commented 8 years ago

Please be respectful, there's no need to be rude. Obviously Number#valueOf would not fail on a true number object. But, how would you know if you can return a primitive or an object, in sloppy mode? Number.call(Object(2), 2) === 2 && Number.call(Object(3), 2) === 2 must both pass. Both receivers will be valid number objects that valueOf works on and are instanceof Number.

zloirock commented 8 years ago

Sorry if offended, I'm just tired. Sloppe mode does not change here anything. An object will be returned on something like Number.call(Object.create(Number.prototype), 2). Possible cover and this case too, but I think it makes no sense.

ljharb commented 8 years ago

It changes whether this can be a primitive value. In sloppy mode, all "this" values are passed through ToObject.

zloirock commented 8 years ago

But this will not be Number instance.

ljharb commented 8 years ago

In sloppy mode, (function () { return typeof this; }(3)) will be "object" - it definitely will be a Number instance. And, in both strict and sloppy mode, passing Object(3) in as "this" will be a Number instance.

zloirock commented 8 years ago

Number.call(Object(3), 2) returns primitive. Number.call(3, 2) too. Number.call(Object.create(Number.prototype), 2) - like wrapper case - not. Thats all.

ljharb commented 8 years ago

I guess I just don't understand how Number.call(Object(2), 2) can know to return a primitive and also new Number(2) to return an object. I'll keep playing with it and see what I can figure out.

ljharb commented 8 years ago

I've gotten all tests to pass except when a Number object is passed as the receiver that has the same primitive value as the first argument to the constructor. I can't conceive of any way without new.target to cover this case.

ljharb commented 8 years ago

I also fixed my wrapConstructor implementation per your suggestion; so binary and octal number strings will now work in ES3.

ljharb commented 8 years ago

I believe the only remaining issue now from your original post is typeof Number.call(Object(3), 3) === 'number' failing - but I suspect this will fail with core-js as well. The skipped test is here.

I'll reopen this issue if core-js has a way to pass this test - otherwise, I think we're in parity and at the limits of shimmability for this.

zloirock commented 8 years ago

but I suspect this will fail with core-js as well

default

ljharb commented 8 years ago

Thanks for confirming - I'll reopen.

I honestly have no idea how this works, but if it's possible without new.target, I need to handle it as well.

ljharb commented 8 years ago

k, thanks - I figured it out.

Appreciate the reports and the followthrough!

zloirock commented 8 years ago

BTW as I wrote above, .bind-based way implementation [[Call]] and [[Construct]] without new.target much more beauty and little more correct. Simple example:

function Internal(){
  return this instanceof Internal ? Object.create(Public.prototype) : 42;
}
var Public = Internal.bind(null);
Public.prototype = {constructor: Public};

console.log(new Public); // => object
console.log(Public()); // => 42
console.log(Public.call(new Public)) // => 42;
console.log(Public.call(Object.create(Public.prototype))); // => 42

But core-js can't use it in this case because of it should work in ES3 environment (looks like not principal for you), .name and .length incorrect and in simple case it's slower.

ljharb commented 8 years ago

Ideally the es6-shim works in an es5-shimed ES3 environment. I can use bind since es5-shim can shim it.

Thanks for explaining the bind approach - that might be worth doing, since even though it's slower, constructing numbers from non-literals is very rare.

zloirock commented 8 years ago

I thought you just disabled ES3 support, but you included it again. So all critical bugs still in IE8- - NFE and lost properties.

default

ljharb commented 8 years ago

Latest fix released in v0.33.9.

zloirock commented 8 years ago

I wrote it already 4 or 5 times, but the first bug still not fixed - Number constructor constants still removed in ES3 engines.

ljharb commented 8 years ago

@zloirock I haven't found any evidence of that but I'm probably missing something. Would you mind filing a new issue detailing which constants are removed in ES3 engines?