down-level of object spread for V8 targets not consistent with ES

[kzc]

The recent change b1a394b to down-level object spread for V8 based targets is not consistent with ES.

Example:

$ cat object-spread-get-set.js
console.log(JSON.stringify({
    get p() {
        console.log("getter");
        return this.q / 2;
    },
    set p(v) {
        console.log("setter", v);
        this.q = v * 2;
    },
    ...{ p: 2 }
}));

Expected result:

$ cat object-spread-get-set.js | node-v14.16.0 
{"p":2}

The result above matches Chrome, Firefox and the @babel/plugin-proposal-object-rest-spread default setting of loose: false.

Actual esbuild result:

$ cat object-spread-get-set.js | esbuild --target=node14 | node-v14.16.0 
setter 2
getter
{"p":2,"q":4}

The incorrect result matches Babel's "loose" mode for an ES6 target which also uses Object.assign.

If the Object.assign behavior is desirable for object spreads then this issue could be resolved with documentation alone.

Here's an example adapted from the Babel test suite highlighting other corner cases of object spread:

$ cat o.js
/*
adapted from:

https://github.com/babel/babel/blob/master/packages/babel-plugin-proposal-object-rest-spread/test/fixtures/object-spread/no-object-assign-exec/exec.js
*/

"use strict";
const NOSET = `NOSET`;
const NOWRITE = `NOWRITE`;

Object.defineProperty(Object.prototype, NOSET, {
  set(value) {
  },
});

Object.defineProperty(Object.prototype, NOWRITE, {
  writable: false,
  value: 'abc',
});

try {
  const obj = { [NOSET]: 123 };
  // this wouldn't work as expected if transformed as Object.assign (or equivalent)
  // because those trigger object setters (spread don't)
  const objSpread = { ...obj };

  const obj2 = { NOSET: 123, [NOWRITE]: 456 };
  // this line would throw `TypeError: Cannot assign to read only property 'NOWRITE'`
  // if transformed as Object.assign (or equivalent) because those use *assignment* for creating properties
  // (spread defines them)
  const obj2Spread = { ...obj2 };

  console.log(1, JSON.stringify(objSpread) === JSON.stringify(obj));
  console.log(2, JSON.stringify(obj2Spread) === JSON.stringify(obj2));
} catch (ex) {
  console.log(1, ex.message);
}

const KEY = Symbol('key');
const obj3Spread = { ...{ get foo () { return 'bar' } }, [KEY]: 'symbol' };
console.log(3, Object.getOwnPropertyDescriptor(obj3Spread, 'foo').value === 'bar');
console.log(4, Object.getOwnPropertyDescriptor(obj3Spread, KEY).value === 'symbol');

const obj4Spread = { ...Object.prototype };
console.log(5, Object.getOwnPropertyDescriptor(obj4Spread, 'hasOwnProperty') === undefined);

try {
  console.log(6, JSON.stringify({ ...null, ...undefined }) === '{}');
} catch (ex) {
  console.log(6, ex.message);
}

const o = Object.create(null);
o.a = 'foo';
o.__proto__ = [];
const o2 = { ...o };
console.log(7, Array.isArray(Object.getPrototypeOf(o2)) === false);

Expected:

$ cat o.js | node-v14.16.0 
1 true
2 true
3 true
4 true
5 true
6 true
7 true

Actual:

$ cat o.js | esbuild --target=node14 | node-v14.16.0 
1 Cannot assign to read only property 'NOWRITE' of object '#<Object>'
3 true
4 true
5 true
6 true
7 false

[evanw]

Thanks for the heads up. I think being correct would be best. My biggest concern is causing a performance regression, since this is very performance-sensitive (see #951).

However, my manual implementation of copying properties over one-by-one is actually faster than Object.assign. I didn't expect that. So I guess I should go ahead with the manual implementation.

Here's my straightforward implementation in esbuild's existing runtime code style (i.e. optimizing for minified code size):

var __defProp = Object.defineProperty;
var __hasOwnProp = Object.prototype.hasOwnProperty;
var __getOwnPropSymbols = Object.getOwnPropertySymbols;
var __propIsEnum = Object.prototype.propertyIsEnumerable;

var __defNormalProp = (obj, key, value) => key in obj
  ? __defProp(obj, key, {enumerable: true, configurable: true, writable: true, value})
  : obj[key] = value;

var __assign = (a, b) => {
  b || (b = {});
  for (var prop in b)
    if (__hasOwnProp.call(b, prop))
      __defNormalProp(a, prop, b[prop]);
  if (__getOwnPropSymbols)
    for (var prop of __getOwnPropSymbols(b)) {
      if (__propIsEnum.call(b, prop))
        __defNormalProp(a, prop, b[prop]);
    }
  return a;
};

Running that through the benchmark in #951 (comment) gives results that look roughly like this:

• Using Object.assign:

  objectSpread 3507ms
  objectSpreadAssign 1308ms
  objectRest 1452ms
  objectRestAssign 848ms
  

• Using the __assign implementation above:

  objectSpread 3594ms
  objectSpreadAssign 1125ms
  objectRest 1618ms
  objectRestAssign 578ms