Review notes

[anba]

http://ljharb.github.io/proposal-object-values-entries/#EnumerableOwnProperties

Step 5.a.iii.2.a is redundant, key is always a string.

The polyfill needs to use Object.getOwnPropertyNames and Object.prototype.propertyIsEnumerable instead of Object.keys and Object.prototype.hasOwnProperty to be compliant to the specification text.

And there is also a typo in Object.entries, [[k, O[k]] should be [k, O[k]]:

const ownNames = Object.getOwnPropertyNames;
const reduce = Function.bind.call(Function.call, Array.prototype.reduce);
const isEnumerable = Function.bind.call(Function.call, Object.prototype.propertyIsEnumerable);
const concat = Function.bind.call(Function.call, Array.prototype.concat);

if (!Object.values) {
    Object.values = function values(O) {
        return reduce(ownNames(O), (v, k) => concat(v, isEnumerable(O, k) ? [O[k]] : []), []);
    };
}

if (!Object.entries) {
    Object.entries = function entries(O) {
        return reduce(ownNames(O), (e, k) => concat(e, isEnumerable(O, k) ? [k, O[k]] : []), []);
    };
}

[ljharb]

Thanks for the thorough review!

Step 5.a.iii.2.a is redundant, key is always a string.

[[OwnPropertyKeys]] can also return Symbol keys (step 4) so this step is required - it's also present in EnumerableOwnNames step 5.a for the same reason.

And there is also a typo in Object.entries, [[k, O[k]] should be [k, O[k]]:

Because I'm using concat, which flattens arrays it receives, the double-array is required, or else it will flatten to an array of alternating keys and values.

The polyfill needs to use Object.getOwnPropertyNames and Object.prototype.propertyIsEnumerable instead of Object.keys and Object.prototype.hasOwnProperty to be compliant to the specification text.

Certainly if the polyfill uses getOwnPropertyNames, I'd need propertyIsEnumerable - however, by using Object.keys I avoid the need for that, and then only need to ensure the property still exists.

How would it be observable whether I'm using Object.keys, Object.getOwnPropertyNames, or Object.getOwnPropertySymbols? Am I missing some Proxy trap possibilities (I'm still fuzzy on those)? The polyfill need not follow every spec step as long as it achieves identical observable semantics.

[anba]

Step 5.a.iii.2.a is redundant, key is always a string.

[[OwnPropertyKeys]] can also return Symbol keys (step 4) so this step is required - it's also present in EnumerableOwnNames step 5.a for the same reason.

Step 5.a "If Type(key) is String, then " is required, but I was referring to step 5.a.iii.2.a "Let elementKey be ToString(key).". This step is redundant because "key" is always a string (per step 5.a).

And there is also a typo in Object.entries, [[k, O[k]] should be [k, O[k]]:

Because I'm using concat, which flattens arrays it receives, the double-array is required, or else it will flatten to an array of alternating keys and values.

Yes, I understand why a double array is needed here, but nonetheless there's a extraneous left bracket which needs to be removed. 😄

/tmp/polyfill.js:14:68 SyntaxError: missing ] after element list:
/tmp/polyfill.js:14:68  reduce(keys(O), (e, k) => concat(e, has(O, k) ? [[k, O[k]] : []), []);
/tmp/polyfill.js:14:68 ............................................................^

The polyfill needs to use Object.getOwnPropertyNames and Object.prototype.propertyIsEnumerable instead of Object.keys and Object.prototype.hasOwnProperty to be compliant to the specification text.

Certainly if the polyfill uses getOwnPropertyNames, I'd need propertyIsEnumerable - however, by using Object.keys I avoid the need for that, and then only need to ensure the property still exists.

How would it be observable whether I'm using Object.keys, Object.getOwnPropertyNames, or Object.getOwnPropertySymbols? Am I missing some Proxy trap possibilities (I'm still fuzzy on those)? The polyfill need not follow every spec step as long as it achieves identical observable semantics.

The difference is visible with the test case from https://bugzilla.mozilla.org/show_bug.cgi?id=1208464#c13:

// Expected: Returns ["A"]
// Actual: Returns ["A", "B"]
Object.values({
  get a() {
    Object.defineProperty(this, "b", {enumerable: false});
    return "A";
  },
  b: "B"
});

[ljharb]

aha, thanks for clarifying. I wasn't referencing elementKey anywhere anyways. I'll remove.

you're right that the brackets don't match up - I actually am missing the matching right bracket. Will fix.

Thanks for the test case! I'll update that in the included polyfill (and the separate shims) shortly, and close this issue when I do.

[anba]

you're right that the brackets don't match up - I actually am missing the matching right bracket. Will fix.

Ah sure. Maybe I should have read the code more carefully... -_-

[zloirock]

Agree about propertyIsEnumerable in the polyfill, but Object.getOwnPropertyNames currently creates more serious problem with the property order. Although mostly in old engines.

[ljharb]

OK - that should cover all the issues you brought up. Please reopen, or file new ones, as you see fit! :-D

[zloirock]

@ljharb this case will not be passed with for-in / keys w/o getOwnPropertyNames:

Object.values(Object.defineProperty({
  get a() {
    Object.defineProperty(this, 'b', {enumerable: true, value: 'C'});
    return 'A';
  }
}, 'b', {value: 'B', writable: true, configurable: true}));

[ljharb]

That test case isn't actually guaranteed to pass by the spec. There's a specific note already in the spec: "any added property during enumeration is not guaranteed to be included in the output" - I'll refine this to be "any property added or marked enumerable during enumeration" to be clearer.

[zloirock]

There's a specific note already in the spec

For [[Enumerate]]. But [[OwnPropertyKeys]] returns all own keys, 'b' present, but non-enumerable, on [[OwnPropertyKeys]] call moment. By current spec it should be passed.

I have no ideas how to fix this case with correct property order in all engines %) I think, I'll just replace hasOwnProperty -> propertyIsEnumerable in my polyfills.

[ljharb]

If there's value in specifying it, I'm open to discussing that in a new issue and doing so - but I think it's fine to not overly proscribe the edge case of modifying properties mid-enumeration.