ljharb / object.assign

ES6 spec-compliant Object.assign shim. From https://github.com/es-shims/es6-shim
https://tc39.es/ecma262/#sec-object.assign
MIT License
107 stars 22 forks source link

Enumeration order in V8 #15

Closed zloirock closed 8 years ago

zloirock commented 8 years ago

Node 4.2.1:

// test.js
require('object.assign').shim()
var t = {};
'abcdefghijklmnopqrst'.split('').forEach(_ => t[_] = _);
console.log(Object.keys(Object.assign({}, t)).join(''));
//...
node test
// => ctnodlqrsmighbjkapfe
node test
// => othknjeacsfdipbglmrq
node test
// => ibkfoclatenhqrgsjpmd

Why? It's a bug of V8 implementation - the same Object.getOwnPropertyNames enumeration order. Required runtime test, but you don't do it. You test only implementation, not result of require('object.assign').shim(). Related https://github.com/kangax/compat-table/commit/afa89886cdf8a954f5572b3c96f50122582c9a08

ljharb commented 8 years ago

Thanks for the report!

It looks like the implementation of this is actually correct, because it's using object-keys and not Object.keys or for..in. A simpler test case that avoids Object.keys is test.js having:

require('object.assign').shim();
var t = {};
'abcdefghijklmnopqrst'.split('').forEach(_ => t[_] = _);
var str = '';
var obj = Object.assign({}, t);
for (var k in obj) {
  str += k;
}
console.log(str);

However, despite the implementation being correct, the shim isn't currently detecting this case. I'll get a fix up for this shortly.

ljharb commented 8 years ago

Also, interesting to note that 'abcdefghijklmnopqrst' triggers the bug, but 'abcdefghijklmnopqrs' does not!

ljharb commented 8 years ago

Released as v4.0.2.