Object.defineProperty does not fail silently in IE8

[schmod]

The documentation for es5-sham seems to imply that Object.defineProperty() should fail when setting properties such as configurable, enumerable, and writable.

However, running the following code throws an error:

Object.defineProperty({foo: 'bar'}, 'foo', {
   configurable: true
})'

Looking at the relevant portion of the sham, it seems like we almost always throw an error if getters/setters are unsupported (even if we're not asking for a getter or setter):

Object.defineProperty = function defineProperty(object, property, descriptor) {
    if ((typeof object !== 'object' && typeof object !== 'function') || object === null) {
        throw new TypeError(ERR_NON_OBJECT_TARGET + object);
    }
    if ((typeof descriptor !== 'object' && typeof descriptor !== 'function') || descriptor === null) {
        throw new TypeError(ERR_NON_OBJECT_DESCRIPTOR + descriptor);
    }
    // make a valiant attempt to use the real defineProperty
    // for I8's DOM elements.
    if (definePropertyFallback) {
        try {
            return definePropertyFallback.call(Object, object, property, descriptor);
        } catch (exception) {
            // try the shim if the real one doesn't work
        }
    }

    // If it's a data property.
    if ('value' in descriptor) {
        // fail silently if 'writable', 'enumerable', or 'configurable'
        // are requested but not supported
        /*
        // alternate approach:
        if ( // can't implement these features; allow false but not true
            ('writable' in descriptor && !descriptor.writable) ||
            ('enumerable' in descriptor && !descriptor.enumerable) ||
            ('configurable' in descriptor && !descriptor.configurable)
        ))
            throw new RangeError(
                'This implementation of Object.defineProperty does not support configurable, enumerable, or writable.'
            );
        */

        if (supportsAccessors && (lookupGetter(object, property) || lookupSetter(object, property))) {
            // As accessors are supported only on engines implementing
            // `__proto__` we can safely override `__proto__` while defining
            // a property to make sure that we don't hit an inherited
            // accessor.
            /*eslint-disable no-proto */
            var prototype = object.__proto__;
            object.__proto__ = prototypeOfObject;
            // Deleting a property anyway since getter / setter may be
            // defined on object itself.
            delete object[property];
            object[property] = descriptor.value;
            // Setting original `__proto__` back now.
            object.__proto__ = prototype;
            /*eslint-enable no-proto */
        } else {
            object[property] = descriptor.value;
        }
    } else {
        if (!supportsAccessors) {
            throw new TypeError(ERR_ACCESSORS_NOT_SUPPORTED);
        }
        // If we got that far then getters and setters can be defined !!
        if ('get' in descriptor) {
            defineGetter(object, property, descriptor.get);
        }
        if ('set' in descriptor) {
            defineSetter(object, property, descriptor.set);
        }
    }
    return object;
};

The above code always throws ERR_ACCESSORS_NOT_SUPPORTED instead of silently failing (which is the documented behavior).

[schmod]

See also: #5.

[hazzik]

In the worst of circumstances, IE 8 provides a version of this method that only works on DOM objects. This sham will not be installed. The given version of defineProperty will throw an exception if used on non-DOM objects.

[ljharb]

This is likely just an oversight over time since 2011 when it was first written. In IE 8, which provides a half-broken Object.defineProperty, for the sham of course we should attempt to provide as much functionality as possible. So, it should install the sham so that it works with writable/configurable/enumerable properties as well as DOM objects, and either fail silently, or throw, when attempting to make a non-writable/non-configurable/non-enumerable property.

[lyfeyaj]

Any updates for this issue?

[ljharb]

This is related to #249 as well.

No updates yet - pull requests are welcome. The sham provides zero guarantees about its behavior across various browsers, so it doesn't tend to get worked on as quickly, but I'd be happy to review something :-)

[pedroteixeira]

Any workarounds for es5-sham throwing on IE8?

[schmod]

I can take a stab at it, but I don't necessarily understand the entire rationale behind the current code path.

There's some (commented-out) code in there that would handle this use-case, but it would need to move, and I'm not sure why it was originally commented out...

[ljharb]

Hopefully @WebReflection can comment.

@schmod The primary concern is that as many browsers as possible have a conforming defineProperty shim, and if it can't be fully compliant, the caveats (as a sham) need to be well understood and documented.

Please feel free to submit a PR!

[WebReflection]

unless I am misreading something, if ('value' in descriptor) guards against that throw and assign ... what is the scenario when you don't have value, neither get nor set ? If it's about updating the writable or the enumerable or the configurable bit then yes, we've got a problem, and that check should be

if (('get' in descriptor || 'set' in descriptor) && !supportsAccessors) throw ...

But then since IE8 won't be able to update any part of the descriptor in regular objects, I am not sure what should happen at the end ... probably nothing, if silent failure is expected.

[pkese]

Just to share my experience... I have recently moved away from es5-shim to core-js for shimming (core-js is also included in babel and its browser-polyfill) and in my case, it works very well (I use it with react and react-router).

[ljharb]

core-js doesn't guarantee 100% spec-compliance, as I understand it.

[ljharb]

There's no need for +1's on this.

The question is, when using the es5-sham (meaning, you've accepted certain caveats), if you try to use Object.defineProperty without providing a value in a browser where getters/setters aren't supported, what should happen?

As @WebReflection points out in #297 (comment), what would you expect the behavior to be if you're simply attempting to update configurability, writability, or enumerability?

Certainly, if it throws, it shouldn't throw an "accessors not supported" error, but a more descriptive one.

So, the decision to make here: fix the error message and update the documentation, or remove the error in this case and comply with the documented behavior?

[ljharb]

#315 now makes it so it only throws when accessors aren't supported if you provide "get" or "set".

The remaining open question: should https://github.com/yiminghe/es5-shim/blob/definePropertyFix/es5-sham.js#L383-L395 be uncommented (and moved up above the if/else) so that defineProperty throws when you attempt to set impossible properties?

[schmod]

I think it makes sense to uncomment those lines, unless there's some weird edge case that we're not thinking of (which caused them to be commented out in the first place).

[Reggino]

@pkese thanks for the tip, I'm running into the same issues with the same setup. However, switching to core-js didn't fix it for me, running into the exact same stuff... ([email protected] throws "Accessors not supported!")

I guess i'll have to update my app to react-router@1-beta

[dutchcelt]

I have the same problem and had to resort to loading es5-sham with a document.write() to make it work.

[cdaloisio]

@dutchcelt could you please elaborate further on how you got this to work using document.write()?

[ljharb]

The issue is that react-router is not compatible with IE 8, before v1.0. However, v0.13.4 is likely to be released soon which will fix this.

I can't possible conceive of any situation where document.write is a good choice.

[schmod]

Hm. Thinking more about this, uncommenting those lines appears to violate the spirit of the sham.

I think it makes sense for us to throw when getters and setters are requested, because there's very little to gain if we fail silently in those places (ie. there's a very high probability that the underlying application will stop working).

On the other hand, configurable and writable are comparable to Object.seal() and Object.freeze() which only provide safeguards, and are rarely used in practice. In these cases, I think that silent failure is appropriate for the sham (albeit a terrible practice for production applications).

enumerable is the the only place where I have major reservations about the current behavior, as anybody who has a good reason for setting enumerable: false is going to encounter bugs if that action fails silently.

However, I think that this is a separate issue. If we can confirm that #315 resolved the original issue, I think that we can close this ticket. Not quite sure what @dutchcelt was encountering, because the behavior documented in my original post appears to have been corrected.