kolodny/jsan

False circular reference positive for repeated reference

Closed this issue · 4 comments

Ran into a surprising circular reference error when using Redux DevTools Extension. This happened when my state included a repeated reference, under a similar key name. There were other circular references in the object which appeared to be correctly identified.

Here's a minimal reproduction example, the problem is that the repeated ref in "myDataAgain" is identified as circular, but there is no circular reference here:

import jsan from "jsan";

const myCircularRef = {};
myCircularRef.myself = myCircularRef;

const data = [{}];

const state = {
  myCircularRef, // A real error is necessary to reproduce the issue
  myData: data, // This is fine
  myDataAgain: data, // This appears as "[CIRCULAR]"
  aDifferentRefToData: data // This is fine
};

const result = jsan.stringify(state, null, 4, { circular: "[CIRCULAR]" });
console.log(result);

Which produces this output:

{
    "myCircularRef": {
        "myself": "[CIRCULAR]"
    },
    "myData": [
        {}
    ],
    "myDataAgain": "[CIRCULAR]",
    "aDifferentRefToData": {
        "$jsan": "$.myData"
    }
} 

The following conditions seem necessary:

  1. A real error of some sort in the object being serialized (there's an intentional circular reference here)
  2. A repeated reference, where the key associated with the second reference contains the key of the first reference ("myDataAgain" contains "myData")

If you change any of these things, the repeated ref is no longer identified as being circular, and serializes as "aDifferentRefToData" does.

Interesting, it looks like

if (path.indexOf(foundPath) === 0) {

should change to be

if (path.indexOf === foundPath) {

I'm not sure if or how that would break the existing behavior. You can create a PR for this if you'd like (including a test case), or I can create a fix for this when I get a chance.

Thanks for reporting and good catch!

I think I have a viable PR here - all I've done is ignore the final component of the current path, since that seems like that's what tripping it up here. All existing tests pass and I've added a TDD test to confirm this fixes the issue as observed.

(I believe this was fixed by #31 and can be closed.)

Yup, thanks!