Top level force decirc

[bengourley]

As discussed briefly here, if an object with forceDecirc = true is passed as the top-level object to fss(obj) it does not have any effect.

This PR adds a failing test for this behaviour and then provides the fix.

[davidmarkclements]

this seems reasonable.. out of interest can I see benchmarks

I've been burnt a few times by changing this lib, and because of the amount of dependents I'm super reluctant to change things (even as a bug fix) which might lead to bugs for other people

it absolutely seems fine, but I'm still .. scared

@mcollina when you have time (which you don't right now) could you weigh in

[BridgeAR]

This is obsolete with the soon to be released v.2 because there is no need for the forceDecirc property anymore. Thanks a lot for the PR though @bengourley

Do we want to add this to v.1.x? @davidmarkclements

@mcollina if this breaks pino-noir, something is bad in pino because it would mean you add the toJSON function including the extra property and expect it not to be looked at. So that would be a bug in pino and not here.

[mcollina]

pino-noir was written with this logic in mind, truly leveraging the internal traversal done here. It might still work ;)

[BridgeAR]

@bengourley would you be so kind and rebase? :-)

[bengourley]

I just has a quick look at doing this, but it turns out the changes that have happened on master appear to make the test I added on this branch pass without the change I made to index.js!

All that's needed is this patch, if you still want the test?

diff --git a/test.js b/test.js
index 111c799..983cddb 100644
--- a/test.js
+++ b/test.js
@@ -236,3 +236,24 @@ test('nested child circular reference in toJSON', function (assert) {
   assert.is(actual, expected)
   assert.end()
 })
+
+test('forceDecirc works on top level object passed to stringify', function (assert) {
+  var circ = {}
+  circ.circ = circ
+  var o = {
+    circ: circ,
+    toJSON: function () {
+      return { circ: this.circ, y: 20 }
+    }
+  }
+  o.toJSON.forceDecirc = true
+  var expected = s({
+    circ: {
+      circ: '[Circular]'
+    },
+    y: 20
+  })
+  var actual = fss(o)
+  assert.is(actual, expected)
+  assert.end()
+})

[bengourley]

(Also thanks for #25, we're using this module on bugsnag-js so IE8 support is crucial 👍)

[BridgeAR]

Ah, yes, I also see what change made your test pass!

Sure, we can add the test :-) It will be removed in v.2 though as the flag is not necessary from then on anymore.

[bengourley]

Ok – will close this now. Thanks!