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

Object.defineProperties(obj, { constructor: .. }} broken in IE8 #252

Closed
Krinkle opened this issue Jun 18, 2014 · 5 comments · Fixed by #305
Closed

Object.defineProperties(obj, { constructor: .. }} broken in IE8 #252

Krinkle opened this issue Jun 18, 2014 · 5 comments · Fixed by #305
Labels

Comments

@Krinkle
Copy link
Contributor

Krinkle commented Jun 18, 2014

Properties other than constructor seem to work fine. And when using Object.defineProperty directly it works fine as well.

I ran into this when using Object.create to set up a subclass prototype and restoring the constructor property afterwards:

    targetFn.prototype = Object.create( originFn.prototype, {
        // Restore constructor property of targetFn
        constructor: {
            value: targetConstructor,
            enumerable: false,
            writable: true,
            configurable: true
        }
    } );

In modern browsers (e.g. latest Chrome) without the es5-shim, targetFn.prototype.constructor === targetConstructor. But in IE8 the defineProperties call was a no-op.

// Windows 7 / MSIE 8.0 (browserstack.com)
// Blank html file with es5-shim.js and es5-sham.js (v3.4.0)

var a = {};
a.foo = 1;
Object.defineProperty(a, 'foo', { value: 3 });
a.foo;
//> 2
Object.defineProperty(a, 'constructor', { value: String });
a.constructor + '';
//> function String() { [native code] }

var b = {};
b.foo = 1;
Object.defineProperties(b, { foo: { value: 2 }});
b.foo;
//> 2
Object.defineProperties(b, { constructor: { value: String }});
b.constructor + '';
//> function Object() { [native code] }

// ^ Property was not updated
@ljharb
Copy link
Member

ljharb commented Jun 18, 2014

IE 8 doesn't support property definitions in any way - you'll note that Object.defineProperty is in the es5-sham, which only does a best-effort attempt to shim things. In addition, please note that Object.create(null) can't work in IE 8.

Please read the readme for the caveats on the es5-sham methods.

@ljharb ljharb closed this as completed Jun 18, 2014
@Krinkle
Copy link
Contributor Author

Krinkle commented Jun 18, 2014

I'm familiar with the sham principle and best-effort attempt it does.

In the library this is for we make a principle to (at the moment) only use ES5 features that can be relatively easily functionally polyfilled back to IE7. We don't use property descriptors with getters/setters, and no Object.create(null).

Property descriptors don't matter much as they're deterministic in other browsers (like strict mode). If the application depends on it, one is probably doing it wrong. It's more to get better feedback during development to avoid doing something you didn't intend. Having it just set the property value in older browsers seems perfectly possible. And is in fact what Object.defineProperty does already. It just doesn't work when triggering it via Object.defineProperties or Object.create( .., ).

I think it's reasonable to aim for consistency within the sham. Either decide property descriptors should be ignored always or used always. Or maybe only when they have a value property. I don't think the current situation was intentional and would hope it's relatively trivial to straighten out (though I couldn't find it).

@Krinkle
Copy link
Contributor Author

Krinkle commented Jun 18, 2014

Figured out why it wasn't working. In old IE, the DontEum / enumerable:false behaviour of a property is infectious and overrides it in any objects that inherits the object where such property exists.

Thus, a plain object like { constructor: 123 } yields no iterations when looped over with for-in, because the constructor property of Object.prototype is unenumerable.

See also:

@Krinkle
Copy link
Contributor Author

Krinkle commented Jun 18, 2014

The fix for it has already been implemented in es5-shim.js (not sham!) for Object.keys which is subject to the same bug naturally.

@ljharb ljharb reopened this Jun 18, 2014
@ljharb
Copy link
Member

ljharb commented Jun 18, 2014

If you can provide a test case, I'll be happy to fix/accept a PR fixing the sham.

Whether it's reasonable or not to be able to ignore descriptors is a decision for each library, and a very subjective one - the ES shims' position on this is that if it doesn't work 100% or 0% in all browsers, it must not be in the shim.

andybry pushed a commit to andybry/es5-shim that referenced this issue May 30, 2015
In IE8 the 'constructor' property was not being
defined by the Object.defineProperties method
of the sham because IE8 does not enumerate the
constructor property.
ljharb added a commit that referenced this issue May 30, 2015
IE8 - Fix #252 'constructor' in Object.defineProperties
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants