Fix #44 Add support for circular getter references

[kristofferremback]

Hello!

Fixes #44

In short, this fixes an issue where circular references in getters weren't replaced in decirc which threw the errorTypeError: Converting circular structure to JSON when eventually passed to JSON.stringify. To not have that problem, I remove the property and set it to '[Circular]' since straight up setting it won't work (unless it has a defined setter).

Example object that threw before:

const o = {
  a: 1,
  get self() {
    return o
  },
}

Let me know if there's anything else I can do to try to remediate this. :)

I ran the benchmarks which yielded the below results. I wouldn't trust them fully though as I'd have expected my changes to have been objectively slower on all cases? Results seems to be close enough though, I believe?

Before:

util.inspect:          simple object x 43,740 ops/sec ±0.45% (88 runs sampled)
util.inspect:          circular      x 21,473 ops/sec ±0.64% (92 runs sampled)
util.inspect:          deep          x 3,067 ops/sec ±0.77% (93 runs sampled)
util.inspect:          deep circular x 3,054 ops/sec ±0.59% (92 runs sampled)

json-stringify-safe:   simple object x 335,428 ops/sec ±1.22% (92 runs sampled)
json-stringify-safe:   circular      x 148,669 ops/sec ±0.74% (90 runs sampled)
json-stringify-safe:   deep          x 8,256 ops/sec ±1.04% (91 runs sampled)
json-stringify-safe:   deep circular x 8,160 ops/sec ±0.66% (90 runs sampled)

fast-safe-stringify:   simple object x 1,140,411 ops/sec ±0.62% (92 runs sampled)
fast-safe-stringify:   circular      x 604,163 ops/sec ±0.67% (93 runs sampled)
fast-safe-stringify:   deep          x 38,762 ops/sec ±0.88% (91 runs sampled)
fast-safe-stringify:   deep circular x 38,023 ops/sec ±0.64% (94 runs sampled)

Fastest is
fast-safe-stringify:   simple object

---

After:

util.inspect:          simple object x 45,068 ops/sec ±0.53% (86 runs sampled)
util.inspect:          circular      x 22,214 ops/sec ±0.82% (88 runs sampled)
util.inspect:          deep          x 3,239 ops/sec ±0.70% (93 runs sampled)
util.inspect:          deep circular x 3,191 ops/sec ±0.93% (92 runs sampled)

json-stringify-safe:   simple object x 347,556 ops/sec ±0.70% (92 runs sampled)
json-stringify-safe:   circular      x 154,494 ops/sec ±0.68% (91 runs sampled)
json-stringify-safe:   deep          x 8,541 ops/sec ±0.64% (94 runs sampled)
json-stringify-safe:   deep circular x 8,342 ops/sec ±0.76% (91 runs sampled)

fast-safe-stringify:   simple object x 1,144,744 ops/sec ±0.51% (92 runs sampled)
fast-safe-stringify:   circular      x 557,971 ops/sec ±0.97% (93 runs sampled)
fast-safe-stringify:   deep          x 38,949 ops/sec ±0.60% (92 runs sampled)
fast-safe-stringify:   deep circular x 37,684 ops/sec ±0.65% (92 runs sampled)

Fastest is
fast-safe-stringify:   simple object

[davidmarkclements]

@kristofferostlund thanks for much for this PR, really appreciate you highlighting this

1. I agree the getter should be replaced with [Circular] in the output (also we should avoid delete as that will put the object into slow property mode)
2. I agree decirc should be synced
3. I think we need benchmarks for objects with lots of properties (and lots of get properties) to really see the impact of this change

[kristofferremback]

Sorry, I should have been more explicit, I did mark getters in decirc as [Circular]. I also synced deterministicDecirc so they behave the same.

I tried different ways of marking the getters as [Circular] and found Object.defineProperty(parent, k, { value: '[Circular]' }) to be most efficient.

I tried the following other approaches and found them all marginally slower:

// Redefining the getter
if (Object.getOwnPropertyDescriptor(parent, k).get !== undefined) {
  Object.defineProperty(parent, k, { get: () => '[Circular]' })
} else {
  parent[k] = '[Circular]'
}

// Un-getterify and set the value
if (Object.getOwnPropertyDescriptor(parent, k).get !== undefined) {
  Object.defineProperty(parent, k, { value: undefined, writable: true, enumerable: true })
}
parent[k] = '[Circular]'

// Deleting the property before marking it as [Circular]
if (Object.getOwnPropertyDescriptor(parent, k).get !== undefined) {
  delete parent[k]
}
parent[k] = '[Circular]'

// And then different ways of checking if it is a getter such as
// checking if the get is a function
if (typeof Object.getOwnPropertyDescriptor(parent, k).get === 'function') {
  ...
}

// Or checking if it's writable, though this could trigger a setters functionality
// and potentially break stuff
if (!Object.getOwnPropertyDescriptor(parent, k).writable) {
  ...
}

---

I had to modify the benchmarks when adding larger objects with circular getters since util.inspect() doesn't go the full depth (and different Node version it seems to go to different depths by defualt) and thus rendered unfair benchmarks:

util.inspect:          simple object               x 43,694 ops/sec ±1.06% (90 runs sampled)
util.inspect:          circular                    x 22,233 ops/sec ±0.66% (92 runs sampled)
util.inspect:          circular getters            x 21,869 ops/sec ±0.72% (92 runs sampled)
util.inspect:          deep                        x 1,132 ops/sec ±0.69% (93 runs sampled)
util.inspect:          deep circular               x 1,126 ops/sec ±0.54% (92 runs sampled)
util.inspect:          large deep circular getters x 851 ops/sec ±0.55% (94 runs sampled)

json-stringify-safe:   simple object               x 346,326 ops/sec ±1.24% (91 runs sampled)
json-stringify-safe:   circular                    x 153,442 ops/sec ±0.61% (93 runs sampled)
json-stringify-safe:   circular getters            x 153,272 ops/sec ±0.86% (92 runs sampled)
json-stringify-safe:   deep                        x 8,476 ops/sec ±0.69% (91 runs sampled)
json-stringify-safe:   deep circular               x 8,296 ops/sec ±0.85% (92 runs sampled)
json-stringify-safe:   large deep circular getters x 300 ops/sec ±0.96% (88 runs sampled)

fast-safe-stringify:   simple object               x 1,137,839 ops/sec ±0.71% (92 runs sampled)
fast-safe-stringify:   circular                    x 549,733 ops/sec ±0.72% (91 runs sampled)
fast-safe-stringify:   circular getters            x 555,157 ops/sec ±0.70% (92 runs sampled)
fast-safe-stringify:   deep                        x 38,223 ops/sec ±0.65% (92 runs sampled)
fast-safe-stringify:   deep circular               x 37,554 ops/sec ±0.51% (92 runs sampled)
fast-safe-stringify:   large deep circular getters x 1,335 ops/sec ±0.98% (91 runs sampled)

Fastest is
fast-safe-stringify:   simple object

[kristofferremback]

I don't think my changes are causing Travis to fail. It looks like tap's ESM transform somehow misses the closing brackets (}})) when running Node.js 4.x?

[davidmarkclements]

I don't think my changes are causing Travis to fail. It looks like tap's ESM transform somehow misses the closing brackets (}})) when running Node.js 4.x?

feel free to change tap to --no-esm - we don't need it

[kristofferremback]

I've gotten non-configurable circular getters to work now.

But I found an issue where circular references returned not as the main value, but as a property of an object is not marked as circular since I believe we now mark the getter's return value's value as [Circular], which is problematic, right?

To clarify:

// Won't work for now
const o = {
  a: 1,
  get o() {
    return { o: o }
  }
}

I'll look into this tomorrow after work again.

---

Also: this time I think I did break Travis on Node.js 4.x haha!

[kristofferremback]

Sorry for the slow finish here, I've been ill basically all week. I solved circular references in child elements and added some tests for it.

util.inspect:          simple object                  x 44,952 ops/sec ±1.15% (88 runs sampled)
util.inspect:          circular                       x 22,454 ops/sec ±0.72% (89 runs sampled)
util.inspect:          circular getters               x 22,529 ops/sec ±0.73% (94 runs sampled)
util.inspect:          deep                           x 1,076 ops/sec ±0.73% (92 runs sampled)
util.inspect:          deep circular                  x 1,066 ops/sec ±0.56% (92 runs sampled)
util.inspect:          large deep circular getters    x 815 ops/sec ±1.99% (92 runs sampled)
util.inspect:          deep non-conf circular getters x 1,071 ops/sec ±0.73% (92 runs sampled)

json-stringify-safe:   simple object                  x 344,785 ops/sec ±0.81% (89 runs sampled)
json-stringify-safe:   circular                       x 152,935 ops/sec ±0.82% (91 runs sampled)
json-stringify-safe:   circular getters               x 153,137 ops/sec ±0.84% (89 runs sampled)
json-stringify-safe:   deep                           x 8,042 ops/sec ±0.67% (94 runs sampled)
json-stringify-safe:   deep circular                  x 7,861 ops/sec ±1.01% (91 runs sampled)
json-stringify-safe:   large deep circular getters    x 285 ops/sec ±0.79% (86 runs sampled)
json-stringify-safe:   deep non-conf circular getters x 7,427 ops/sec ±0.92% (93 runs sampled)

fast-safe-stringify:   simple object                  x 1,152,173 ops/sec ±0.63% (91 runs sampled)
fast-safe-stringify:   circular                       x 559,578 ops/sec ±0.86% (90 runs sampled)
fast-safe-stringify:   circular getters               x 559,467 ops/sec ±0.70% (92 runs sampled)
fast-safe-stringify:   deep                           x 37,123 ops/sec ±1.14% (89 runs sampled)
fast-safe-stringify:   deep circular                  x 36,857 ops/sec ±0.69% (92 runs sampled)
fast-safe-stringify:   large deep circular getters    x 1,313 ops/sec ±0.56% (91 runs sampled)
fast-safe-stringify:   deep non-conf circular getters x 10,669 ops/sec ±0.98% (93 runs sampled)

Fastest is
fast-safe-stringify:   simple object

[kristofferremback]

Hi there! What is the plan to have this merged and published? Anything I can help with?

[davidmarkclements]

published as 2.0.7

[kristofferremback]

Awesome, thank you!