Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Polyfilled properties should not be enumerable when possible #83

Closed
Yaffle opened this Issue · 16 comments

6 participants

@Yaffle

https://github.com/kriskowal/es5-shim/blob/master/es5-shim.js#L1058
Native trim property of String.prototype replaced with enumerable property
for(var i in 'test') {
if (i === 'trim') throw "bug!";
}

@domenic
Collaborator

In general it would be nice to make the shims non-enumerable where possible, for the cases where a browser supports Object.defineProperty but is missing or has buggy implementations of the shimmed function.

Possible helpers here: https://github.com/DomenicDenicola/es6-shim/blob/master/es6-shim.js

@Yaffle

form other side it is better, when code work same in all supported browsers...
so replacing all native "trim" will be good

@derekrprice

Is this why jQuery 1.10.2 $.each doesn't work properly with es5-shim? It looks like it is iterating over the methods defined on Object in es5-shim.js (forEach, reduceRight, ...)

@Yaffle

@jdalton , ok
i did not know, that assigment preserver [[Enumerable]] and other flags
so this issue can be closed

@Yaffle Yaffle closed this
@hazzik

Why is this closed?

I think the issue shall be reopened and we shell try to use Object.defineProperty where supported.

Also it is related to #93

@ljharb
Owner

@hazzik due to limitations in IE 6-8, it's impossible to ever handle this in any consistent way, and the consensus is that an inconsistent solution is worse than none at all.

@hazzik

I mean for normal browsers/js-engines, where we have proper Object.defineProperty we sometimes add or replace native functions with polyfills.

For example phantomjs which does not have Function.prototype.bind but have Object.defineProperty and so bind become enumerable.

Or node 0.6.21, where we replace Array's functions with polyfills

@hazzik

So with following implementation of defineProperty function the results will be consistent between Object.keys and for ... in:

var defineProperty = Object.defineProperty;
try { 
    // check for IE8
    defineProperty && defineProperty({}, 'answer', { value: 42 });
} catch (e) {
    defineProperty = undefined;
}
defineProperty = defineProperty || function (obj, prop, descriptor) {
    obj[prop] = descriptor.value;
}
@ljharb
Owner

Ah, you're saying that instead of always using assignment, we should be using a conditional defineProperty method that falls back to simple assignment. I'll reopen.

@ljharb ljharb reopened this
@ljharb ljharb changed the title from enumerable properties to Polyfilled properties should not be enumerable when possible
@Yaffle

@hazzik

simple assigment preserves [[Enumerable]]:

var object = {};
Object.defineProperty(object, "property1", {
  value: 42,
  enumerable: false,
  writable: true,
  configurable: true
});
object.property1 = 43;
console.log(Object.getOwnPropertyDescriptor(object, "property1").enumerable);
@ljharb
Owner

Only if it's initially non-enumerable, though, right?

@hazzik

@Yaffle, can you confirm that this is a standard behaviour? Even so, it would work only for overridden methods when we fix behaviour, but would not work for new methods, like Function.prototype.bind in phantomjs

@Yaffle

@hazzik , yes, i do not use for-in for functions and for strings too, so that issue is not interesting for me

@hazzik

@Yaffle what about Arrays? ;)

@ljharb ljharb added this to the v3.1.0 milestone
@ljharb ljharb referenced this issue from a commit in ljharb/es5-shim
@ljharb ljharb Refactor to use internal defineProperties method.
Fixes #83
d864678
@ljharb ljharb referenced this issue from a commit in ljharb/es5-shim
@ljharb ljharb Refactor to use internal defineProperties method.
Fixes #83
4539695
@ljharb ljharb closed this in #250
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.