Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Number constructor issues #365

Closed
zloirock opened this issue Oct 21, 2015 · 26 comments
Closed

Number constructor issues #365

zloirock opened this issue Oct 21, 2015 · 26 comments
Assignees

Comments

@zloirock
Copy link

  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
Copy link
Collaborator

ljharb commented Oct 21, 2015

_.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
Copy link
Collaborator

ljharb commented Oct 22, 2015

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 :-)

ljharb added a commit that referenced this issue Oct 22, 2015
@ljharb ljharb closed this as completed in 1327dcc Oct 22, 2015
@ljharb ljharb self-assigned this Oct 22, 2015
@zloirock
Copy link
Author

+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
Copy link
Collaborator

ljharb commented Oct 23, 2015

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

@zloirock
Copy link
Author

@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
Copy link
Collaborator

ljharb commented Oct 23, 2015

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
Copy link
Author

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

@ljharb
Copy link
Collaborator

ljharb commented Oct 23, 2015

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
Copy link
Author

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
Copy link
Collaborator

ljharb commented Oct 23, 2015

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

@zloirock
Copy link
Author

But this will not be Number instance.

@ljharb
Copy link
Collaborator

ljharb commented Oct 23, 2015

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
Copy link
Author

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
Copy link
Collaborator

ljharb commented Oct 23, 2015

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
Copy link
Collaborator

ljharb commented Oct 23, 2015

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
Copy link
Collaborator

ljharb commented Oct 23, 2015

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

@ljharb
Copy link
Collaborator

ljharb commented Oct 23, 2015

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
Copy link
Author

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

default

@ljharb
Copy link
Collaborator

ljharb commented Oct 23, 2015

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 ljharb reopened this Oct 23, 2015
@ljharb ljharb closed this as completed in 8add25f Oct 23, 2015
@ljharb
Copy link
Collaborator

ljharb commented Oct 23, 2015

k, thanks - I figured it out.

Appreciate the reports and the followthrough!

@zloirock
Copy link
Author

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
Copy link
Collaborator

ljharb commented Oct 23, 2015

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
Copy link
Author

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 added a commit to es-shims/es6-shim that referenced this issue Oct 27, 2015
@ljharb
Copy link
Collaborator

ljharb commented Oct 29, 2015

Latest fix released in v0.33.9.

@zloirock
Copy link
Author

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

@ljharb
Copy link
Collaborator

ljharb commented Jan 26, 2016

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants