Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strip circular references #7

Closed
zalmoxisus opened this issue Nov 20, 2016 · 7 comments
Closed

Strip circular references #7

zalmoxisus opened this issue Nov 20, 2016 · 7 comments

Comments

@zalmoxisus
Copy link
Collaborator

While it's great that jsan can restore circular references, in many cases we want them stripped as it will affect rendering perf and could hang object traversing.

It would be useful to have an additional parameter to use it like so:

jsan.parse(obj, null, '[CIRCULAR]');

Or if it is more performant we could strip them while stringifying. I couldn't wrap my head around it, but it seems that it would be easier to do while parsing inside retrocycle.

@kolodny
Copy link
Owner

kolodny commented Nov 20, 2016

This is already possible as follows:

var jsan = require('jsan')

var obj = {}
obj.a = 1
obj.self = obj;

var set = new WeakSet();
var stringified = jsan.stringify(obj, (key, value) => {
  if (set.has(value)) return '[CIRCULAR]';;
  if (typeof value === 'object') set.add(value);
  return value;
});

console.log(stringified)

Hmm, I can't seem to save on http://requirebin.com/ but you can just copy paste it to see for yourself

Does that solve your issue?

@zalmoxisus
Copy link
Collaborator Author

zalmoxisus commented Nov 20, 2016

I guess we obtain almost the same as just jsan.stringify(obj) (and then parsing with JSON.parse instead of jsan.parse), I don't mind showing $jsan instead of [CIRCULAR], though it was a bit confusing.

The problem however is that in case of

var o = { t: 2 };
var obj = { e1: o, e2: o}
obj.a = 1
obj.self = obj;
console.log(customJsanStringify(obj)

we get {"e1":{"t":2},"e2":"[CIRCULAR]","a":1,"self":"[CIRCULAR]"}, but e2 is not a circular reference.

We should also account if we're including the parent, which maybe better to handle in decycle, as we have already there the structure? Or maybe when restoring the structure back from jsan.parse?

@kolodny
Copy link
Owner

kolodny commented Nov 21, 2016

Hmm, that's not a trivial problem. I guess we can detect something like that by checking that path.indexOf(foundPath) === 0 by

var foundPath = map.get(value);
but I'm not sold on baking that into jsan. Can you think of a less hacky implementation?

@zalmoxisus
Copy link
Collaborator Author

zalmoxisus commented Nov 21, 2016

I'll try to convince you :-D

The idea is that most of the time people get circular references because they are including React components inside the object (accidentally or intended), and they are not interested to see the whole circular structure of that component. We have to remove them everytime we want to show the state or comparing states. Removing them outside jsan will be less performant.

If we implement it as an option, it shouldn't affect the perf of the default implementation:

      if (typeof value === 'object' && value !== null) {
+        if (options['circular'] === false && path.indexOf(foundPath) === 0) {
+          return '[CIRCULAR]';
+        }
        var foundPath = map.get(value);
        if (foundPath) {
          return {$jsan: foundPath};
        }
        map.set(value, path);
      }

Or to gain some perf from not replacing paths inside jsan.parse in this case:

      if (typeof value === 'object' && value !== null) {
+        if (options['circular'] === false) {
+          if (path.indexOf(foundPath) === 0) {
+            return '[CIRCULAR]';
+          }
+        } else {
        var foundPath = map.get(value);
        if (foundPath) {
          return {$jsan: foundPath};
        }
+      }
        map.set(value, path);
      }

Or even moving that part inside a function we can call, not to check options['circular'] === false everytime:

      function includePath() {
        if (options['circular'] === false) {
          return function() {
            if (path.indexOf(foundPath) === 0) {
              return '[CIRCULAR]';
            }
          }
        }
        return function() {
          var foundPath = map.get(value);
          if (foundPath) {
            return {$jsan: foundPath};
          }
        }
      }

Of course, there are already solutions like json-stringify-safe (maybe even more performant for this case than checking path.indexOf for every value), but we want all the goodies of jsan.

@kolodny
Copy link
Owner

kolodny commented Nov 22, 2016

Ok, I'm convinced. I've played around with it and it doesn't seem to have any perf hit I want to play around with the api some more first. I'll push a release soon 3dbebf0

@kolodny
Copy link
Owner

kolodny commented Nov 22, 2016

published to 3.1.5. Usage is undocumented but can be seen here:

https://github.com/kolodny/jsan/blob/master/test/unit.js#L77-L85

For your use case you would just pass in a string '[Circular]'

@zalmoxisus
Copy link
Collaborator Author

It works just amazing! Thanks a lot for your effort on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants